[ros-diffs] [reactos] 01/01: [KERNEL32] Minor enhancements for CreateRemoteThread(). (#804)

[Hermès Bélusca-Maïto]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=44cddadba80aa9efab0da…

commit 44cddadba80aa9efab0da36aa57cf6f7a012df43
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu Aug 23 21:44:53 2018 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Thu Aug 1 19:12:49 2019 +0200

    [KERNEL32] Minor enhancements for CreateRemoteThread(). (#804)
    
    - Add some cleanup code in failure code paths, instead of asserting.
    - Move BasepNotifyCsrOfThread() helper to the only file where it is used.
    - Don't use ERROR_DBGBREAK in failure paths but just DPRINT the error
      message: we handle the failures properly.
    - When creating the remote thread, sync its service tag with the parent
      thread's one.
---
 dll/win32/kernel32/client/proc.c   |  30 ----------
 dll/win32/kernel32/client/thread.c | 113 ++++++++++++++++++++++++-------------
 2 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/dll/win32/kernel32/client/proc.c b/dll/win32/kernel32/client/proc.c
index f36219457b2..b44624c381b 100644
--- a/dll/win32/kernel32/client/proc.c
+++ b/dll/win32/kernel32/client/proc.c
@@ -479,36 +479,6 @@ BaseProcessStartup(PPROCESS_START_ROUTINE lpStartAddress)
     _SEH2_END;
 }
 
-NTSTATUS
-WINAPI
-BasepNotifyCsrOfThread(IN HANDLE ThreadHandle,
-                       IN PCLIENT_ID ClientId)
-{
-    BASE_API_MESSAGE ApiMessage;
-    PBASE_CREATE_THREAD CreateThreadRequest = &ApiMessage.Data.CreateThreadRequest;
-
-    DPRINT("BasepNotifyCsrOfThread: Thread: %p, Handle %p\n",
-            ClientId->UniqueThread, ThreadHandle);
-
-    /* Fill out the request */
-    CreateThreadRequest->ClientId = *ClientId;
-    CreateThreadRequest->ThreadHandle = ThreadHandle;
-
-    /* Call CSR */
-    CsrClientCallServer((PCSR_API_MESSAGE)&ApiMessage,
-                        NULL,
-                        CSR_CREATE_API_NUMBER(BASESRV_SERVERDLL_INDEX,
BasepCreateThread),
-                        sizeof(*CreateThreadRequest));
-    if (!NT_SUCCESS(ApiMessage.Status))
-    {
-        DPRINT1("Failed to tell CSRSS about new thread: %lx\n",
ApiMessage.Status);
-        return ApiMessage.Status;
-    }
-
-    /* Return Success */
-    return STATUS_SUCCESS;
-}
-
 BOOLEAN
 WINAPI
 BasePushProcessParameters(IN ULONG ParameterFlags,
diff --git a/dll/win32/kernel32/client/thread.c b/dll/win32/kernel32/client/thread.c
index 3505d4bdae7..be33021fcf3 100644
--- a/dll/win32/kernel32/client/thread.c
+++ b/dll/win32/kernel32/client/thread.c
@@ -18,12 +18,37 @@
 
 typedef NTSTATUS (NTAPI *PCSR_CREATE_REMOTE_THREAD)(IN HANDLE ThreadHandle, IN PCLIENT_ID
ClientId);
 
+/* FUNCTIONS ******************************************************************/
+
 NTSTATUS
 WINAPI
 BasepNotifyCsrOfThread(IN HANDLE ThreadHandle,
-                       IN PCLIENT_ID ClientId);
+                       IN PCLIENT_ID ClientId)
+{
+    BASE_API_MESSAGE ApiMessage;
+    PBASE_CREATE_THREAD CreateThreadRequest = &ApiMessage.Data.CreateThreadRequest;
+
+    DPRINT("BasepNotifyCsrOfThread: Thread: %p, Handle %p\n",
+            ClientId->UniqueThread, ThreadHandle);
+
+    /* Fill out the request */
+    CreateThreadRequest->ClientId = *ClientId;
+    CreateThreadRequest->ThreadHandle = ThreadHandle;
+
+    /* Call CSR */
+    CsrClientCallServer((PCSR_API_MESSAGE)&ApiMessage,
+                        NULL,
+                        CSR_CREATE_API_NUMBER(BASESRV_SERVERDLL_INDEX,
BasepCreateThread),
+                        sizeof(*CreateThreadRequest));
+    if (!NT_SUCCESS(ApiMessage.Status))
+    {
+        DPRINT1("Failed to tell CSRSS about new thread: %lx\n",
ApiMessage.Status);
+        return ApiMessage.Status;
+    }
 
-/* FUNCTIONS ******************************************************************/
+    /* Return Success */
+    return STATUS_SUCCESS;
+}
 
 __declspec(noreturn)
 VOID
@@ -153,12 +178,13 @@ CreateRemoteThread(IN HANDLE hProcess,
     ULONG_PTR Cookie;
     ULONG ReturnLength;
     SIZE_T ReturnSize;
+
     DPRINT("CreateRemoteThread: hProcess: %p dwStackSize: %lu lpStartAddress"
-            ": %p lpParameter: %p, dwCreationFlags: %lx\n", hProcess,
-            dwStackSize, lpStartAddress, lpParameter, dwCreationFlags);
+           ": %p lpParameter: %p, dwCreationFlags: %lx\n", hProcess,
+           dwStackSize, lpStartAddress, lpParameter, dwCreationFlags);
 
     /* Clear the Context */
-    RtlZeroMemory(&Context, sizeof(CONTEXT));
+    RtlZeroMemory(&Context, sizeof(Context));
 
     /* Write PID */
     ClientId.UniqueProcess = hProcess;
@@ -176,14 +202,14 @@ CreateRemoteThread(IN HANDLE hProcess,
         return NULL;
     }
 
-    /* Create Initial Context */
+    /* Create the Initial Context */
     BaseInitializeContext(&Context,
                           lpParameter,
                           lpStartAddress,
                           InitialTeb.StackBase,
                           1);
 
-    /* initialize the attributes for the thread object */
+    /* Initialize the attributes for the thread object */
     ObjectAttributes = BaseFormatObjectAttributes(&LocalObjectAttributes,
                                                   lpThreadAttributes,
                                                   NULL);
@@ -217,10 +243,10 @@ CreateRemoteThread(IN HANDLE hProcess,
         if (!NT_SUCCESS(Status))
         {
             /* Fail */
-            ERROR_DBGBREAK("SXS: %s - Failing thread create because "
-                           "NtQueryInformationThread() failed with status
%08lx\n",
-                           __FUNCTION__, Status);
-            return NULL;
+            DPRINT1("SXS: %s - Failing thread create because "
+                    "NtQueryInformationThread() failed with status %08lx\n",
+                    __FUNCTION__, Status);
+            goto Quit;
         }
 
         /* Allocate the Activation Context Stack */
@@ -228,10 +254,10 @@ CreateRemoteThread(IN HANDLE hProcess,
         if (!NT_SUCCESS(Status))
         {
             /* Fail */
-            ERROR_DBGBREAK("SXS: %s - Failing thread create because "
-                           "RtlAllocateActivationContextStack() failed with status
%08lx\n",
-                           __FUNCTION__, Status);
-            return NULL;
+            DPRINT1("SXS: %s - Failing thread create because "
+                    "RtlAllocateActivationContextStack() failed with status
%08lx\n",
+                    __FUNCTION__, Status);
+            goto Quit;
         }
 
         /* Save it */
@@ -249,15 +275,10 @@ CreateRemoteThread(IN HANDLE hProcess,
         if (!NT_SUCCESS(Status))
         {
             /* Fail */
-            ERROR_DBGBREAK("SXS: %s - Failing thread create because "
-                           "RtlQueryInformationActivationContext() failed with
status %08lx\n",
-                           __FUNCTION__, Status);
-
-            /* Free the activation context stack */
-            // RtlFreeThreadActivationContextStack();
-            RtlFreeActivationContextStack(Teb->ActivationContextStackPointer);
-
-            return NULL;
+            DPRINT1("SXS: %s - Failing thread create because "
+                    "RtlQueryInformationActivationContext() failed with status
%08lx\n",
+                    __FUNCTION__, Status);
+            goto Quit;
         }
 
         /* Does it need to be activated? */
@@ -271,24 +292,21 @@ CreateRemoteThread(IN HANDLE hProcess,
             if (!NT_SUCCESS(Status))
             {
                 /* Fail */
-                ERROR_DBGBREAK("SXS: %s - Failing thread create because "
-                               "RtlActivateActivationContextEx() failed with status
%08lx\n",
-                               __FUNCTION__, Status);
-
-                /* Free the activation context stack */
-                // RtlFreeThreadActivationContextStack();
-                RtlFreeActivationContextStack(Teb->ActivationContextStackPointer);
-
-                return NULL;
+                DPRINT1("SXS: %s - Failing thread create because "
+                        "RtlActivateActivationContextEx() failed with status
%08lx\n",
+                        __FUNCTION__, Status);
+                goto Quit;
             }
         }
+
+        /* Sync the service tag with the parent thread's one */
+        Teb->SubProcessTag = NtCurrentTeb()->SubProcessTag;
     }
 
     /* Notify CSR */
     if (!BaseRunningInServerProcess)
     {
         Status = BasepNotifyCsrOfThread(hThread, &ClientId);
-        ASSERT(NT_SUCCESS(Status));
     }
     else
     {
@@ -304,16 +322,35 @@ CreateRemoteThread(IN HANDLE hProcess,
             {
                 /* Call it instead of going through LPC */
                 Status = CsrCreateRemoteThread(hThread, &ClientId);
-                ASSERT(NT_SUCCESS(Status));
             }
         }
     }
 
+Quit:
+    if (!NT_SUCCESS(Status))
+    {
+        /* Failed to create the thread */
+
+        /* Free the activation context stack */
+        if (ActivationContextStack)
+            RtlFreeActivationContextStack(ActivationContextStack);
+
+        NtTerminateThread(hThread, Status);
+        // FIXME: Wait for the thread to terminate?
+        BaseFreeThreadStack(hProcess, &InitialTeb);
+        NtClose(hThread);
+
+        BaseSetLastNTError(Status);
+        return NULL;
+    }
+
     /* Success */
-    if (lpThreadId) *lpThreadId = HandleToUlong(ClientId.UniqueThread);
+    if (lpThreadId)
+        *lpThreadId = HandleToUlong(ClientId.UniqueThread);
 
-    /* Resume it if asked */
-    if (!(dwCreationFlags & CREATE_SUSPENDED)) NtResumeThread(hThread, &Dummy);
+    /* Resume the thread if asked */
+    if (!(dwCreationFlags & CREATE_SUSPENDED))
+        NtResumeThread(hThread, &Dummy);
 
     /* Return handle to thread */
     return hThread;