[PATCH 0/1] MR4654: Draft: msvcrt: Protect setlocale against concurrent accesses.

[Bernhard Übelacker (＠bernhardu)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55467

[PATCH 1/1] msvcrt: Protect setlocale against concurrent accesses.

[Bernhard Übelacker]

From: Bernhard Übelacker

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();
 }

[Piotr Caban (＠piotr)]

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

[Piotr Caban (＠piotr)]

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).