[PATCH 0/1] MR4654: Draft: msvcrt: Protect setlocale against concurrent accesses.
From: Bernhard Übelacker <bernhardu(a)mailbox.org> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55467 --- dlls/msvcrt/locale.c | 22 ++++++++++++++++------ dlls/msvcrt/tests/locale.c | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/dlls/msvcrt/locale.c b/dlls/msvcrt/locale.c index f7be2a1e9bc..3bfbddbecfb 100644 --- a/dlls/msvcrt/locale.c +++ b/dlls/msvcrt/locale.c @@ -2034,20 +2034,30 @@ _locale_t CDECL _wcreate_locale(int category, const wchar_t *locale) */ char* CDECL setlocale(int category, const char* locale) { - thread_data_t *data = msvcrt_get_thread_data(); - pthreadlocinfo locinfo = get_locinfo(), newlocinfo; + thread_data_t *data; + pthreadlocinfo locinfo, newlocinfo; + + _lock_locales(); + data = msvcrt_get_thread_data(); + locinfo = get_locinfo(); - if(category<LC_MIN || category>LC_MAX) + if(category<LC_MIN || category>LC_MAX) { + _unlock_locales(); return NULL; + } if(!locale) { + char* ret; if(category == LC_ALL) - return construct_lc_all(locinfo); - - return locinfo->lc_category[category].locale; + ret = construct_lc_all(locinfo); + else + ret = locinfo->lc_category[category].locale; + _unlock_locales(); + return ret; } newlocinfo = create_locinfo(category, locale, locinfo); + _unlock_locales(); if(!newlocinfo) { WARN("%d %s failed\n", category, locale); return NULL; diff --git a/dlls/msvcrt/tests/locale.c b/dlls/msvcrt/tests/locale.c index f77e8f93abb..7300ed30803 100644 --- a/dlls/msvcrt/tests/locale.c +++ b/dlls/msvcrt/tests/locale.c @@ -796,6 +796,25 @@ static void test___mb_cur_max_func(void) } } +static DWORD WINAPI test_setlocale_threads_proc(void *arg) +{ + const char* locale = NULL; + if (*(int*)arg % 2) locale = "German_Germany.1252"; + setlocale(LC_NUMERIC, locale); + return 0; +} + +static void test_setlocale_threads(void) +{ + HANDLE threads[100]; + for (int i = 0; i < ARRAY_SIZE(threads); i++) + { + threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)&i, 0, NULL); + ok(threads[i] != NULL, "CreateThread failed\n"); + } + WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE); +} + START_TEST(locale) { init(); @@ -804,4 +823,5 @@ START_TEST(locale) test_setlocale(); test__Gettnames(); test___mb_cur_max_func(); + test_setlocale_threads(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4654
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
> + const char* locale = NULL;
> + if (*(int*)arg % 2) locale = "German_Germany.1252";
> + setlocale(LC_NUMERIC, locale);
> + return 0;
> +}
> +
> +static void test_setlocale_threads(void)
> +{
> + HANDLE threads[100];
> + for (int i = 0; i < ARRAY_SIZE(threads); i++)
> + {
> + threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)&i, 0, NULL);
> + ok(threads[i] != NULL, "CreateThread failed\n");
> + }
> + WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE);
> +}
The test needs more work:
- you should pass `i` as argument (not `&i`)
- ARRAY_SIZE(threads) is bigger than MAXIMUM_WAIT_OBJECTS
- I think the test will be better if only 2 threads are created
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56510
Piotr Caban (@piotr) commented about dlls/msvcrt/locale.c:
*/ char* CDECL setlocale(int category, const char* locale) { - thread_data_t *data = msvcrt_get_thread_data(); - pthreadlocinfo locinfo = get_locinfo(), newlocinfo; + thread_data_t *data; + pthreadlocinfo locinfo, newlocinfo; + + _lock_locales(); While this change works we should limit the time when _lock_locales lock is held. It can be achieved by acquiring the lock, increasing locale refcounts and releasing the lock (there's `grab_locinfo` helper that does that).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56511
participants (2)
-
Bernhard Übelacker -
Piotr Caban (＠piotr)