Draft: msvcrt: Protect setlocale against concurrent accesses.
Merge request reports
Activity
requested review from @piotr
801 const char* locale = NULL; 802 if (*(int*)arg % 2) locale = "German_Germany.1252"; 803 setlocale(LC_NUMERIC, locale); 804 return 0; 805 } 806 807 static void test_setlocale_threads(void) 808 { 809 HANDLE threads[100]; 810 for (int i = 0; i < ARRAY_SIZE(threads); i++) 811 { 812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)&i, 0, NULL); 813 ok(threads[i] != NULL, "CreateThread failed\n"); 814 } 815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE); 816 } Thanks for having a look. The test failing manifests just with a crash. I wanted to have the test crash like the application and took a few threads too much. If I reduce the threads to just two I do no longer receive the crash, with 8 not always, with 16 quite reliable (with a 16 thread cpu).
2034 2034 */ 2035 2035 char* CDECL setlocale(int category, const char* locale) 2036 2036 { 2037 thread_data_t *data = msvcrt_get_thread_data(); 2038 pthreadlocinfo locinfo = get_locinfo(), newlocinfo; 2037 thread_data_t *data; 2038 pthreadlocinfo locinfo, newlocinfo; 2039 2040 _lock_locales(); changed this line in version 2 of the diff
I did some testing to reduce the time and found just using the grab_locinfo makes the crash go away, without using any _lock_locales. Is it then not needed? And from looking at the other grab_locinfo uses I did not see how it gets released later, maybe just by the free_locinfo calls?
added 123 commits
-
942e7be9...91a29134 - 122 commits from branch
wine:master
- 2076f93e - msvcrt: Protect setlocale against concurrent accesses.
-
942e7be9...91a29134 - 122 commits from branch
796 796 } 797 797 } 798 798 799 static DWORD WINAPI test_setlocale_threads_proc(void *arg) 800 { 801 const char* locale = NULL; 802 if ((intptr_t)arg % 2) locale = "German_Germany.1252"; 803 setlocale(LC_NUMERIC, locale); - Comment on lines +801 to +803
It still crashes for me when
setlocale(LC_NUMERIC, NULL)
calls are removed. It even seems to crash more often.
800 { 801 const char* locale = NULL; 802 if ((intptr_t)arg % 2) locale = "German_Germany.1252"; 803 setlocale(LC_NUMERIC, locale); 804 return 0; 805 } 806 807 static void test_setlocale_threads(void) 808 { 809 HANDLE threads[64]; 810 for (intptr_t i = 0; i < ARRAY_SIZE(threads); i++) 811 { 812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL); 813 ok(threads[i] != NULL, "CreateThread failed\n"); 814 } 815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE); - Comment on lines +809 to +815
809 HANDLE threads[64]; 810 for (intptr_t i = 0; i < ARRAY_SIZE(threads); i++) 811 { 812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL); 813 ok(threads[i] != NULL, "CreateThread failed\n"); 814 } 815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE); 809 HANDLE threads[64]; 810 int i; 811 812 for (i = 0; i < ARRAY_SIZE(threads); i++) 813 { 814 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL); 815 ok(threads[i] != NULL, "CreateThread failed\n"); 816 } 817 for (i = 0; i < ARRAY_SIZE(threads); i++) 818 { 819 WaitForSingleObject(threads[i], INFINITE); 820 CloseHandle(threads[i]); 821 }
2047 2049 return locinfo->lc_category[category].locale; 2048 2050 } 2049 2051 2050 2052 newlocinfo = create_locinfo(category, locale, locinfo); After thinking about it more it would be even better to do something like:
2052 newlocinfo = create_locinfo(category, locale, locinfo); 2052 /* Make sure that locinfo is not updated by e.g. stricmp function */ 2053 locale_flags = data->locale_flags; 2054 data->locale_flags |= LOCALE_THREAD; 2055 newlocinfo = create_locinfo(category, locale, locinfo); 2056 data->locale_flags = locale_flags; This way all the helper functions will be called with the same locale.
mentioned in merge request !4816 (closed)
mentioned in merge request !4829 (merged)
The patch was merged in !4829 (merged). Thank you.