[PATCH 1/2] msvcrt: Increase module's reference count before returning from _beginthread[ex]().

[Arkadiusz Hiler]

Increasing DLL's reference count from the trampoline function makes it
prone to race conditions. The thread can start executing after we have
already returned from _beginthread[ex]() and the DLL might have been
freed.

Fixes rare crash on launch with Baldur's Gate 3.

Signed-off-by: Arkadiusz Hiler 
---
 dlls/msvcrt/thread.c | 50 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/dlls/msvcrt/thread.c b/dlls/msvcrt/thread.c
index 01500d93d91..173763d0eb1 100644
--- a/dlls/msvcrt/thread.c
+++ b/dlls/msvcrt/thread.c
@@ -32,6 +32,9 @@ typedef struct {
       _beginthreadex_start_routine_t start_address_ex;
   };
   void *arglist;
+#if _MSVCR_VER >= 140
+  HMODULE module;
+#endif
 } _beginthread_trampoline_t;

/*********************************************************************
@@ -113,16 +116,10 @@ static DWORD CALLBACK _beginthread_trampoline(LPVOID arg)
     thread_data_t *data = msvcrt_get_thread_data();

memcpy(&local_trampoline,arg,sizeof(local_trampoline));
-    data->handle = local_trampoline.thread;
     free(arg);
-
+    data->handle = local_trampoline.thread;
 #if _MSVCR_VER >= 140
-    if (!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
-                (void*)local_trampoline.start_address, &data->module))
-    {
-        data->module = NULL;
-        WARN("failed to get module for the start_address: %d\n", GetLastError());
-    }
+    data->module = local_trampoline.module;
 #endif

local_trampoline.start_address(local_trampoline.arglist);
@@ -162,7 +159,19 @@ uintptr_t CDECL _beginthread(
   trampoline->start_address = start_address;
   trampoline->arglist = arglist;

+#if _MSVCR_VER >= 140
+  if(!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
+              (void*)start_address, &trampoline->module))
+  {
+      trampoline->module = NULL;
+      WARN("failed to get module for the start_address: %d\n", GetLastError());
+  }
+#endif
+
   if(ResumeThread(thread) == -1) {
+#if _MSVCR_VER >= 140
+      FreeLibrary(trampoline->module);
+#endif
       free(trampoline);
       *_errno() = EAGAIN;
       return -1;
@@ -181,19 +190,10 @@ static DWORD CALLBACK _beginthreadex_trampoline(LPVOID arg)
     thread_data_t *data = msvcrt_get_thread_data();

memcpy(&local_trampoline, arg, sizeof(local_trampoline));
-    data->handle = local_trampoline.thread;
     free(arg);
-
+    data->handle = local_trampoline.thread;
 #if _MSVCR_VER >= 140
-    {
-        thread_data_t *data = msvcrt_get_thread_data();
-        if (!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
-                    (void*)local_trampoline.start_address_ex, &data->module))
-        {
-            data->module = NULL;
-            WARN("failed to get module for the start_address: %d\n", GetLastError());
-        }
-    }
+    data->module = local_trampoline.module;
 #endif

retval = local_trampoline.start_address_ex(local_trampoline.arglist);
@@ -225,9 +225,21 @@ uintptr_t CDECL _beginthreadex(
   trampoline->start_address_ex = start_address;
   trampoline->arglist = arglist;

+#if _MSVCR_VER >= 140
+  if(!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
+              (void*)start_address, &trampoline->module))
+  {
+     trampoline->module = NULL;
+     WARN("failed to get module for the start_address: %d\n", GetLastError());
+  }
+#endif
+
   thread = CreateThread(security, stack_size, _beginthreadex_trampoline,
           trampoline, initflag, thrdaddr);
   if(!thread) {
+#if _MSVCR_VER >= 140
+      FreeLibrary(trampoline->module);
+#endif
       free(trampoline);
       msvcrt_set_errno(GetLastError());
       return 0;

[PATCH 2/2] ucrtbase/tests: Test for FreeLibrary() vs _beginthread[ex]() race condition.

[Arkadiusz Hiler]

Signed-off-by: Arkadiusz Hiler 
---
 dlls/ucrtbase/tests/thread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dlls/ucrtbase/tests/thread.c b/dlls/ucrtbase/tests/thread.c
index 8fffbffb38e..7693304e612 100644
--- a/dlls/ucrtbase/tests/thread.c
+++ b/dlls/ucrtbase/tests/thread.c
@@ -105,11 +105,11 @@ static void test_thread_library_reference(char *thread_dll,

ok(thread_handle != -1 && thread_handle != 0, "Failed to begin thread: %u\n", errno);

+    ok(FreeLibrary(dll), "Failed to free the library: %u\n", GetLastError());
+
     ret = WaitForSingleObject(args.confirm_running, 200);
     ok(ret == WAIT_OBJECT_0, "Event was not signaled, ret: %u, err: %u\n", ret, GetLastError());

-    ok(FreeLibrary(dll), "Failed to free the library: %u\n", GetLastError());
-
     ret = WaitForSingleObject(detach_event, 0);
     ok(ret == WAIT_TIMEOUT, "Thread detach happened unexpectedly signaling an event, ret: %d, err: %u\n", ret, GetLastError());

[PATCH 2/2] ucrtbase/tests: Test for FreeLibrary() vs _beginthread[ex]() race condition.

[Piotr Caban]

Signed-off-by: Piotr Caban

[Piotr Caban]

Signed-off-by: Piotr Caban