[PATCH v2 0/1] MR6644: Draft: msvcrt: Initialize locale data in new threads.

[Bernhard Übelacker (＠bernhardu)]

This attempts to initialize the thread data more like in the mainthread,
which does in msvcrt_init_locale a call to _setmbcp, which finally
fills the cached_locale member of the thread data.

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

--
  v2: msvcrt: Initialize locale data in new threads.

https://gitlab.winehq.org/wine/wine/-/merge_requests/6644

[PATCH v2 1/1] msvcrt: Initialize locale data in new threads.

[Bernhard Übelacker]

From: Bernhard Übelacker

This attempts to initialize the thread data more like in the mainthread,
which does in msvcrt_init_locale a call to _setmbcp, which finally
fills the cached_locale member of the thread data.

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

diff --git a/dlls/msvcrt/tests/locale.c b/dlls/msvcrt/tests/locale.c
index cb56fe39754..70496b9efcf 100644
--- a/dlls/msvcrt/tests/locale.c
+++ b/dlls/msvcrt/tests/locale.c
@@ -19,6 +19,7 @@
  */

#include <locale.h>
+#include <process.h>

#include "wine/test.h"
 #include "winnls.h"
@@ -845,6 +846,29 @@ static void test__wcsicmp_l(void)
     }
 }

+static void __cdecl test_thread_setlocale_func(void * arg __attribute__((unused)))
+{
+    char *ret;
+
+    ret = setlocale(LC_ALL, NULL);
+    ok(!strcmp(ret, "C"), "expected ret=C, but received ret=%s\n", ret);
+
+    ret = setlocale(LC_ALL, "");
+    ok(strcmp(ret, "Invariant Language_Invariant Country.0"), "expected something different, but received ret=%s\n", ret);
+
+    _endthread();
+}
+
+static void test_thread_setlocale(void)
+{
+    HANDLE hThread;
+
+    hThread = (HANDLE)_beginthread(test_thread_setlocale_func, 0, NULL);
+    ok(hThread != INVALID_HANDLE_VALUE, "_beginthread failed (%d)\n", errno);
+    WaitForSingleObject(hThread, 5000);
+    CloseHandle(hThread);
+}
+
 START_TEST(locale)
 {
     init();
@@ -854,4 +878,5 @@ START_TEST(locale)
     test__Gettnames();
     test___mb_cur_max_func();
     test__wcsicmp_l();
+    test_thread_setlocale();
 }
diff --git a/dlls/msvcrt/thread.c b/dlls/msvcrt/thread.c
index 6aad9a22d47..bd0214029bf 100644
--- a/dlls/msvcrt/thread.c
+++ b/dlls/msvcrt/thread.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
  */
 #include <process.h>
+#include <mbctype.h>
 #include "msvcrt.h"
 #include "wine/debug.h"

@@ -60,6 +61,7 @@ thread_data_t *CDECL msvcrt_get_thread_data(void)
 #if _MSVCR_VER >= 140
         ptr->module = NULL;
 #endif
+        _setmbcp(_MB_CP_ANSI);
     }
     SetLastError( err );
     return ptr;

[Bernhard Übelacker (＠bernhardu)]

v2:
- Fix build-clang error in thread function: omitting the parameter name in a function definition is a C2x extension

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
>  }
>
> }
>
> +static void __cdecl test_thread_setlocale_func(void * arg __attribute__((unused)))
> +{
> - char *ret;
> -
> - ret = setlocale(LC_ALL, NULL);
> - ok(!strcmp(ret, "C"), "expected ret=C, but received ret=%s\n", ret);
> -
> - ret = setlocale(LC_ALL, "");
> - ok(strcmp(ret, "Invariant Language_Invariant Country.0"), "expected something different, but received ret=%s\n", ret);

Could you please change the error string to something like: `"expected valid locale\n"`? It's already known what's the value of ret if the test fails.

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
>  }
>
> }
>
> +static void __cdecl test_thread_setlocale_func(void * arg __attribute__((unused)))
> +{
> - char *ret;
> -
> - ret = setlocale(LC_ALL, NULL);
> - ok(!strcmp(ret, "C"), "expected ret=C, but received ret=%s\n", ret);
> -
> - ret = setlocale(LC_ALL, "");
> - ok(strcmp(ret, "Invariant Language_Invariant Country.0"), "expected something different, but received ret=%s\n", ret);
> -
> - _endthread();

There's no need to call `_endthread()` here.

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/thread.c:
> #if _MSVCR_VER >= 140
>          ptr->module = NULL;
>  #endif
> -
>    _setmbcp(_MB_CP_ANSI);

You can't call _setmbcp() here - it might modify other thread mbcp when global (non per-thread) settings are used. The bug is caused by using uninitialized locale cache. The simplest solution is to set:
```c
ptr->cached_locale[0] = 'C';
ptr->cached_locale[1] = 0;
```

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
> - char *ret;
> -
> - ret = setlocale(LC_ALL, NULL);
> - ok(!strcmp(ret, "C"), "expected ret=C, but received ret=%s\n", ret);
> -
> - ret = setlocale(LC_ALL, "");
> - ok(strcmp(ret, "Invariant Language_Invariant Country.0"), "expected something different, but received ret=%s\n", ret);
> -
> - _endthread();
>
> +}
> -
>
> +static void test_thread_setlocale(void)
> +{
> - HANDLE hThread;
> -
> - hThread = (HANDLE)_beginthread(test_thread_setlocale_func, 0, NULL);

Please use `_beginthreadex` here. Otherwise the thread handle will be closed when the thread finishes.

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/tests/locale.c:
>  }
>
> }
>
> +static void __cdecl test_thread_setlocale_func(void * arg __attribute__((unused)))

__attribute__((unused)) is probably not portable, please remove it.