[PATCH 2/4] msvcrt: Use trampoline for _beginthreadex().

[Arkadiusz Hiler]

On Mon, May 03, 2021 at 04:24:40PM +0300, Dmitry Timoshkov wrote:
> Arkadiusz Hiler <ahiler at codeweavers.com> wrote:
> 
> > +  thread = CreateThread(security, stack_size, _beginthreadex_trampoline,
> > +          trampoline, initflag | CREATE_SUSPENDED, thrdaddr);
> 
> What's the reason of unconditionally creating a suspended thread?

It's a leftover from initial copy-and-paste of the _beginthread()'s
trampoline logic.

The thread there is suspended because _endthread() closes the handle,
and we only get that after creation.

Here I was doing something simillar initially, but it turned out that
even _endthread() should not close anything. I've changed a few things
but forgot to change this part.

Revised version of the patch is attached, thanks.

-- 
Cheers,
Arek
-------------- next part --------------
>From 4855c53370ef8539d67e551e3a9d2ce395a92a6d Mon Sep 17 00:00:00 2001
From: Arkadiusz Hiler <ahiler at codeweavers.com>
Date: Mon, 3 May 2021 12:04:07 +0300
Subject: [PATCH v2 2/4] msvcrt: Use trampoline for _beginthreadex().

This way we can call _endthreadex() at the end as stated in the documentation.

This will also simplify FreeLibrary()-safe implementation for UCRT.

Thread handle in TLS is set to INVALID_HANDLE_VALUE, so that accidental
_endthread() won't close it, see: existing tests.

Signed-off-by: Arkadiusz Hiler <ahiler at codeweavers.com>
---
 dlls/msvcrt/thread.c | 48 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/dlls/msvcrt/thread.c b/dlls/msvcrt/thread.c
index 650afdc08af..b01e78630a5 100644
--- a/dlls/msvcrt/thread.c
+++ b/dlls/msvcrt/thread.c
@@ -27,7 +27,10 @@ WINE_DEFAULT_DEBUG_CHANNEL(msvcrt);
 
 typedef struct {
   HANDLE thread;
-  _beginthread_start_routine_t start_address;
+  union {
+      _beginthread_start_routine_t start_address;
+      _beginthreadex_start_routine_t start_address_ex;
+  };
   void *arglist;
 } _beginthread_trampoline_t;
 
@@ -146,6 +149,23 @@ uintptr_t CDECL _beginthread(
   return (uintptr_t)thread;
 }
 
+/*********************************************************************
+ *		_beginthreadex_trampoline
+ */
+static DWORD CALLBACK _beginthreadex_trampoline(LPVOID arg)
+{
+    unsigned int retval;
+    _beginthread_trampoline_t local_trampoline;
+    thread_data_t *data = msvcrt_get_thread_data();
+
+    memcpy(&local_trampoline,arg,sizeof(local_trampoline));
+    data->handle = local_trampoline.thread;
+    free(arg);
+
+    retval = local_trampoline.start_address_ex(local_trampoline.arglist);
+
+    _endthreadex(retval);
+}
 /*********************************************************************
  *		_beginthreadex (MSVCRT.@)
  */
@@ -157,12 +177,30 @@ uintptr_t CDECL _beginthreadex(
   unsigned int initflag,   /* [in] Initial state of new thread (0 for running or CREATE_SUSPEND for suspended) */
   unsigned int *thrdaddr)  /* [out] Points to a 32-bit variable that receives the thread identifier */
 {
+  _beginthread_trampoline_t* trampoline;
+  HANDLE thread;
+
   TRACE("(%p, %d, %p, %p, %d, %p)\n", security, stack_size, start_address, arglist, initflag, thrdaddr);
 
-  /* FIXME */
-  return (uintptr_t)CreateThread(security, stack_size,
-				     start_address, arglist,
-				     initflag, thrdaddr);
+  trampoline = malloc(sizeof(*trampoline));
+  if(!trampoline) {
+      *_errno() = EAGAIN;
+      return -1;
+  }
+
+  trampoline->thread = INVALID_HANDLE_VALUE;
+  trampoline->start_address_ex = start_address;
+  trampoline->arglist = arglist;
+
+  thread = CreateThread(security, stack_size, _beginthreadex_trampoline,
+          trampoline, initflag, thrdaddr);
+  if(!thread) {
+      free(trampoline);
+      *_errno() = EAGAIN;
+      return -1;
+  }
+
+  return (uintptr_t)thread;
 }
 
 #if _MSVCR_VER>=80
-- 
2.31.1