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

[Bernhard Übelacker]

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

--
  v2: msvcrt: Protect setlocale against concurrent accesses.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654

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

[Bernhard Übelacker]

From: Bernhard Übelacker <bernhardu(a)mailbox.org>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55467
---
 dlls/msvcrt/locale.c       |  2 ++
 dlls/msvcrt/tests/locale.c | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/dlls/msvcrt/locale.c b/dlls/msvcrt/locale.c
index f7be2a1e9bc..99cd82b29c9 100644
--- a/dlls/msvcrt/locale.c
+++ b/dlls/msvcrt/locale.c
@@ -2037,6 +2037,8 @@ char* CDECL setlocale(int category, const char* locale)
     thread_data_t *data = msvcrt_get_thread_data();
     pthreadlocinfo locinfo = get_locinfo(), newlocinfo;
 
+    grab_locinfo(locinfo);
+
     if(category<LC_MIN || category>LC_MAX)
         return NULL;
 
diff --git a/dlls/msvcrt/tests/locale.c b/dlls/msvcrt/tests/locale.c
index f77e8f93abb..6a6bc5b8cc7 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 ((intptr_t)arg % 2) locale = "German_Germany.1252";
+    setlocale(LC_NUMERIC, locale);
+    return 0;
+}
+
+static void test_setlocale_threads(void)
+{
+    HANDLE threads[64];
+    for (intptr_t 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

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

[Dmitry Timoshkov]

Bernhard Übelacker <wine(a)gitlab.winehq.org> wrote:
> +static void test_setlocale_threads(void)
> +{
> +    HANDLE threads[64];
> +    for (intptr_t 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);
> +}

I'd guess that leaking 64 handles for such a small test is too much.

-- 
Dmitry.

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

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=141539

Your paranoid android.


=== debian11 (32 bit report) ===

msvcr90:
msvcr90.c:2067: Test failed: refcount = 3
msvcr90.c:2078: Test failed: refcount = 2
msvcr90.c:2087: Test failed: refcount = 2
msvcr90.c:2094: Test failed: refcount = 5 for category 2
msvcr90.c:2094: Test failed: refcount = 5 for category 3
msvcr90.c:2094: Test failed: refcount = 5 for category 4
msvcr90.c:2094: Test failed: refcount = 5 for category 5
msvcr90.c:2111: Test failed: refcount = 5
msvcr90.c:2116: Test failed: refcount = 5
msvcr90.c:2121: Test failed: refcount = 5
msvcr90.c:2126: Test failed: refcount = 5
msvcr90.c:2130: Test failed: refcount = 5
msvcr90.c:2137: Test failed: refcount = 2
msvcr90.c:2145: Test failed: refcount = 4 for category 1
msvcr90.c:2147: Test failed: refcount = 2 for category 1
msvcr90.c:2147: Test failed: refcount = 3 for category 2
msvcr90.c:2155: Test failed: refcount = 6 for category 3
msvcr90.c:2155: Test failed: refcount = 6 for category 4
msvcr90.c:2155: Test failed: refcount = 6 for category 5
msvcr90.c:2173: Test failed: refcount = 3
msvcr90.c:2179: Test failed: refcount = 6
msvcr90.c:2184: Test failed: refcount = 6
msvcr90.c:2189: Test failed: refcount = 6
msvcr90.c:2193: Test failed: refcount = 6
msvcr90.c:2200: Test failed: refcount = 2
msvcr90.c:2208: Test failed: refcount = 5 for category 1
msvcr90.c:2210: Test failed: refcount = 2 for category 1
msvcr90.c:2208: Test failed: refcount = 4 for category 2
msvcr90.c:2210: Test failed: refcount = 3 for category 2
msvcr90.c:2210: Test failed: refcount = 4 for category 3
msvcr90.c:2218: Test failed: refcount = 7 for category 4
msvcr90.c:2218: Test failed: refcount = 7 for category 5
msvcr90.c:2236: Test failed: refcount = 3
msvcr90.c:2242: Test failed: refcount = 4
msvcr90.c:2249: Test failed: refcount = 7
msvcr90.c:2254: Test failed: refcount = 4
msvcr90.c:2259: Test failed: refcount = 7
msvcr90.c:2266: Test failed: refcount = 2
msvcr90.c:2274: Test failed: refcount = 6 for category 1
msvcr90.c:2276: Test failed: refcount = 2 for category 1
msvcr90.c:2274: Test failed: refcount = 5 for category 2
msvcr90.c:2276: Test failed: refcount = 3 for category 2
msvcr90.c:2274: Test failed: refcount = 4 for category 3
msvcr90.c:2276: Test failed: refcount = 4 for category 3
msvcr90.c:2276: Test failed: refcount = 5 for category 4
msvcr90.c:2283: Test failed: refcount = 8
msvcr90.c:2300: Test failed: refcount = 3
msvcr90.c:2306: Test failed: refcount = 4
msvcr90.c:2312: Test failed: refcount = 5
msvcr90.c:2318: Test failed: refcount = 4
msvcr90.c:2323: Test failed: refcount = 8
msvcr90.c:2330: Test failed: refcount = 2
msvcr90.c:2338: Test failed: refcount = 7 for category 1
msvcr90.c:2340: Test failed: refcount = 2 for category 1
msvcr90.c:2338: Test failed: refcount = 6 for category 2
msvcr90.c:2340: Test failed: refcount = 3 for category 2
msvcr90.c:2338: Test failed: refcount = 5 for category 3
msvcr90.c:2340: Test failed: refcount = 4 for category 3
msvcr90.c:2338: Test failed: refcount = 4 for category 4
msvcr90.c:2340: Test failed: refcount = 5 for category 4
msvcr90.c:2340: Test failed: refcount = 6 for category 5
msvcr90.c:2358: Test failed: refcount = 3
msvcr90.c:2364: Test failed: refcount = 4
msvcr90.c:2370: Test failed: refcount = 5
msvcr90.c:2376: Test failed: refcount = 4
msvcr90.c:2381: Test failed: refcount = 6
msvcr90.c:2383: Test failed: refcount = 3

=== debian11b (64 bit WoW report) ===

msvcr90:
msvcr90.c:2067: Test failed: refcount = 3
msvcr90.c:2078: Test failed: refcount = 2
msvcr90.c:2087: Test failed: refcount = 2
msvcr90.c:2094: Test failed: refcount = 5 for category 2
msvcr90.c:2094: Test failed: refcount = 5 for category 3
msvcr90.c:2094: Test failed: refcount = 5 for category 4
msvcr90.c:2094: Test failed: refcount = 5 for category 5
msvcr90.c:2111: Test failed: refcount = 5
msvcr90.c:2116: Test failed: refcount = 5
msvcr90.c:2121: Test failed: refcount = 5
msvcr90.c:2126: Test failed: refcount = 5
msvcr90.c:2130: Test failed: refcount = 5
msvcr90.c:2137: Test failed: refcount = 2
msvcr90.c:2145: Test failed: refcount = 4 for category 1
msvcr90.c:2147: Test failed: refcount = 2 for category 1
msvcr90.c:2147: Test failed: refcount = 3 for category 2
msvcr90.c:2155: Test failed: refcount = 6 for category 3
msvcr90.c:2155: Test failed: refcount = 6 for category 4
msvcr90.c:2155: Test failed: refcount = 6 for category 5
msvcr90.c:2173: Test failed: refcount = 3
msvcr90.c:2179: Test failed: refcount = 6
msvcr90.c:2184: Test failed: refcount = 6
msvcr90.c:2189: Test failed: refcount = 6
msvcr90.c:2193: Test failed: refcount = 6
msvcr90.c:2200: Test failed: refcount = 2
msvcr90.c:2208: Test failed: refcount = 5 for category 1
msvcr90.c:2210: Test failed: refcount = 2 for category 1
msvcr90.c:2208: Test failed: refcount = 4 for category 2
msvcr90.c:2210: Test failed: refcount = 3 for category 2
msvcr90.c:2210: Test failed: refcount = 4 for category 3
msvcr90.c:2218: Test failed: refcount = 7 for category 4
msvcr90.c:2218: Test failed: refcount = 7 for category 5
msvcr90.c:2236: Test failed: refcount = 3
msvcr90.c:2242: Test failed: refcount = 4
msvcr90.c:2249: Test failed: refcount = 7
msvcr90.c:2254: Test failed: refcount = 4
msvcr90.c:2259: Test failed: refcount = 7
msvcr90.c:2266: Test failed: refcount = 2
msvcr90.c:2274: Test failed: refcount = 6 for category 1
msvcr90.c:2276: Test failed: refcount = 2 for category 1
msvcr90.c:2274: Test failed: refcount = 5 for category 2
msvcr90.c:2276: Test failed: refcount = 3 for category 2
msvcr90.c:2274: Test failed: refcount = 4 for category 3
msvcr90.c:2276: Test failed: refcount = 4 for category 3
msvcr90.c:2276: Test failed: refcount = 5 for category 4
msvcr90.c:2283: Test failed: refcount = 8
msvcr90.c:2300: Test failed: refcount = 3
msvcr90.c:2306: Test failed: refcount = 4
msvcr90.c:2312: Test failed: refcount = 5
msvcr90.c:2318: Test failed: refcount = 4
msvcr90.c:2323: Test failed: refcount = 8
msvcr90.c:2330: Test failed: refcount = 2
msvcr90.c:2338: Test failed: refcount = 7 for category 1
msvcr90.c:2340: Test failed: refcount = 2 for category 1
msvcr90.c:2338: Test failed: refcount = 6 for category 2
msvcr90.c:2340: Test failed: refcount = 3 for category 2
msvcr90.c:2338: Test failed: refcount = 5 for category 3
msvcr90.c:2340: Test failed: refcount = 4 for category 3
msvcr90.c:2338: Test failed: refcount = 4 for category 4
msvcr90.c:2340: Test failed: refcount = 5 for category 4
msvcr90.c:2340: Test failed: refcount = 6 for category 5
msvcr90.c:2358: Test failed: refcount = 3
msvcr90.c:2364: Test failed: refcount = 4
msvcr90.c:2370: Test failed: refcount = 5
msvcr90.c:2376: Test failed: refcount = 4
msvcr90.c:2381: Test failed: refcount = 6
msvcr90.c:2383: Test failed: refcount = 3

[Bernhard Übelacker]

On Sun Dec 31 11:42:32 2023 +0000, Piotr Caban wrote:
> 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
> 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).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56577

[Bernhard Übelacker]

On Tue Jan  2 16:31:25 2024 +0000, Bernhard Übelacker wrote:
> changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/4654/diffs?diff_id=91693&start_sha=942e7be9d08343f033ad7ec99d4f09ed9c187996#90e640854b517029283ede63e860624a7984b551_2040_2040)
> 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?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56579

[Bernhard Übelacker]

v2:
- Pass i by value to the thread function.
- Use less threads, at least below MAXIMUM_WAIT_OBJECTS.
- Replace _lock_locales by grab_locinfo.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56580

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
> }
>  }
>
> +static DWORD WINAPI test_setlocale_threads_proc(void *arg)
> +{
> +    const char* locale = NULL;
> +    if ((intptr_t)arg % 2) locale = "German_Germany.1252";
> +    setlocale(LC_NUMERIC, locale);

It still crashes for me when `setlocale(LC_NUMERIC, NULL)` calls are removed. It even seems to crash more often.
```suggestion:-2+0
    setlocale(LC_NUMERIC, "German_Germany.1252");
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56832

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/locale.c:
> return locinfo->lc_category[category].locale;
>      }
>
> newlocinfo = create_locinfo(category, locale, locinfo);

```suggestion:-0+0
    /* Make sure that locinfo is not freed when thread locale is updated by e.g. stricmp(). */
    grab_locinfo(locinfo);
    newlocinfo = create_locinfo(category, locale, locinfo);
    free_locinfo(locinfo);
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56835

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/locale.c:
> thread_data_t *data = msvcrt_get_thread_data();
>      pthreadlocinfo locinfo = get_locinfo(), newlocinfo;
>
> +    grab_locinfo(locinfo);
> +

```suggestion:-1+0
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56834

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
> +{
> +    const char* locale = NULL;
> +    if ((intptr_t)arg % 2) locale = "German_Germany.1252";
> +    setlocale(LC_NUMERIC, locale);
> +    return 0;
> +}
> +
> +static void test_setlocale_threads(void)
> +{
> +    HANDLE threads[64];
> +    for (intptr_t 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);
    HANDLE threads[64];
    int i;

    for (i = 0; i &lt; ARRAY_SIZE(threads); i++)
    {
        threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL);
        ok(threads[i] != NULL, "CreateThread failed\n");
    }
    for (i = 0; i &lt; ARRAY_SIZE(threads); i++)
    {
        WaitForSingleObject(threads[i], INFINITE);
        CloseHandle(threads[i]);
    }

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_56833

[Piotr Caban (＠piotr)]

On Sun Jan  7 16:43:44 2024 +0000, Piotr Caban wrote:
> ```suggestion:-0+0
>     /* Make sure that locinfo is not freed when thread locale is updated
> by e.g. stricmp(). */
>     grab_locinfo(locinfo);
>     newlocinfo = create_locinfo(category, locale, locinfo);
>     free_locinfo(locinfo);
> ```

After thinking about it more it would be even better to do something like:
```suggestion:-0+0
    /* Make sure that locinfo is not updated by e.g. stricmp function */
    locale_flags = data->locale_flags;
    data->locale_flags |= LOCALE_THREAD;
    newlocinfo = create_locinfo(category, locale, locinfo);
    data->locale_flags = locale_flags;
```
This way all the helper functions will be called with the same locale.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_57056

[Piotr Caban (＠piotr)]

The patch was merged in !4829. Thank you.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654#note_57315

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

[Piotr Caban (＠piotr)]

This merge request was closed by Piotr Caban.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4654