[ros-diffs] [reactos] 01/01: [KERNEL32][RTL] Diverse fixes/improvements for thread stack creation code. (#802)

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

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

commit effdb6f232aa4448179c2b0a0f70aaa20856357c
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu May 26 04:44:25 2016 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Thu Aug 1 19:04:13 2019 +0200

    [KERNEL32][RTL] Diverse fixes/improvements for thread stack creation code. (#802)
    
    - kernel32!BaseCreateStack() is compatible with ntdll!RtlpCreateUserStack().
    - When checking whether a stack guard page can be added, its size has to
      be accounted for in the checking logic.
    - We have to satisfy the PEB::MinimumStackCommit constraint.
    - We cannot use PEB::GuaranteedStackBytes in BaseCreateStack() since it is
      nowhere initialized (default is 0). It gets initialized to a non-zero
      value when the user manually calls SetThreadStackGuarantee().
     
https://www.installsetupconfig.com/win32programming/windowsthreadsprocessap…
    
    - RtlpCreateUserStack(): Fix memory leak in failure case.
    - RtlpFreeUserStack() doesn't need to return anything.
    
    See also commit 1bc59379 (r59868).
    
    CORE-11319
---
 dll/win32/kernel32/client/thread.c    |  7 +--
 dll/win32/kernel32/client/utils.c     | 68 ++++++++++++++++-------------
 dll/win32/kernel32/include/kernel32.h | 20 +++++----
 sdk/lib/rtl/thread.c                  | 82 ++++++++++++++++++++---------------
 4 files changed, 102 insertions(+), 75 deletions(-)

diff --git a/dll/win32/kernel32/client/thread.c b/dll/win32/kernel32/client/thread.c
index e64cd7230d3..3505d4bdae7 100644
--- a/dll/win32/kernel32/client/thread.c
+++ b/dll/win32/kernel32/client/thread.c
@@ -165,9 +165,10 @@ CreateRemoteThread(IN HANDLE hProcess,
 
     /* Create the Stack */
     Status = BaseCreateStack(hProcess,
-                             dwStackSize,
-                             dwCreationFlags & STACK_SIZE_PARAM_IS_A_RESERVATION ?
-                             dwStackSize : 0,
+                             (dwCreationFlags & STACK_SIZE_PARAM_IS_A_RESERVATION) ?
+                                0 : dwStackSize,
+                             (dwCreationFlags & STACK_SIZE_PARAM_IS_A_RESERVATION) ?
+                                dwStackSize : 0,
                              &InitialTeb);
     if (!NT_SUCCESS(Status))
     {
diff --git a/dll/win32/kernel32/client/utils.c b/dll/win32/kernel32/client/utils.c
index e0654c9700c..6e3a3368ad5 100644
--- a/dll/win32/kernel32/client/utils.c
+++ b/dll/win32/kernel32/client/utils.c
@@ -346,22 +346,25 @@ BaseFormatObjectAttributes(OUT POBJECT_ATTRIBUTES ObjectAttributes,
 }
 
 /*
- * Creates a stack for a thread or fiber
+ * Creates a stack for a thread or fiber.
+ * NOTE: Adapted from sdk/lib/rtl/thread.c:RtlpCreateUserStack().
  */
 NTSTATUS
 WINAPI
-BaseCreateStack(HANDLE hProcess,
-                 SIZE_T StackCommit,
-                 SIZE_T StackReserve,
-                 PINITIAL_TEB InitialTeb)
+BaseCreateStack(
+    _In_ HANDLE hProcess,
+    _In_opt_ SIZE_T StackCommit,
+    _In_opt_ SIZE_T StackReserve,
+    _Out_ PINITIAL_TEB InitialTeb)
 {
     NTSTATUS Status;
     PIMAGE_NT_HEADERS Headers;
     ULONG_PTR Stack;
     BOOLEAN UseGuard;
-    ULONG PageSize, Dummy, AllocationGranularity;
-    SIZE_T StackReserveHeader, StackCommitHeader, GuardPageSize, GuaranteedStackCommit;
-    DPRINT("BaseCreateStack (hProcess: %p, Max: %lx, Current: %lx)\n",
+    ULONG PageSize, AllocationGranularity, Dummy;
+    SIZE_T MinimumStackCommit, GuardPageSize;
+
+    DPRINT("BaseCreateStack(hProcess: 0x%p, Max: 0x%lx, Current: 0x%lx)\n",
             hProcess, StackReserve, StackCommit);
 
     /* Read page size */
@@ -372,34 +375,38 @@ BaseCreateStack(HANDLE hProcess,
     Headers = RtlImageNtHeader(NtCurrentPeb()->ImageBaseAddress);
     if (!Headers) return STATUS_INVALID_IMAGE_FORMAT;
 
-    StackCommitHeader = Headers->OptionalHeader.SizeOfStackCommit;
-    StackReserveHeader = Headers->OptionalHeader.SizeOfStackReserve;
-
-    if (!StackReserve) StackReserve = StackReserveHeader;
+    if (StackReserve == 0)
+        StackReserve = Headers->OptionalHeader.SizeOfStackReserve;
 
-    if (!StackCommit)
+    if (StackCommit == 0)
     {
-        StackCommit = StackCommitHeader;
+        StackCommit = Headers->OptionalHeader.SizeOfStackCommit;
     }
+    /* Check if the commit is higher than the reserve */
     else if (StackCommit >= StackReserve)
     {
+        /* Grow the reserve beyond the commit, up to 1MB alignment */
         StackReserve = ROUND_UP(StackCommit, 1024 * 1024);
     }
 
+    /* Align everything to Page Size */
     StackCommit = ROUND_UP(StackCommit, PageSize);
     StackReserve = ROUND_UP(StackReserve, AllocationGranularity);
 
-    GuaranteedStackCommit = NtCurrentTeb()->GuaranteedStackBytes;
-    if ((GuaranteedStackCommit) && (StackCommit < GuaranteedStackCommit))
+    MinimumStackCommit = NtCurrentPeb()->MinimumStackCommit;
+    if ((MinimumStackCommit != 0) && (StackCommit < MinimumStackCommit))
     {
-        StackCommit = GuaranteedStackCommit;
+        StackCommit = MinimumStackCommit;
     }
 
+    /* Check if the commit is higher than the reserve */
     if (StackCommit >= StackReserve)
     {
+        /* Grow the reserve beyond the commit, up to 1MB alignment */
         StackReserve = ROUND_UP(StackCommit, 1024 * 1024);
     }
 
+    /* Align everything to Page Size */
     StackCommit = ROUND_UP(StackCommit, PageSize);
     StackReserve = ROUND_UP(StackReserve, AllocationGranularity);
 
@@ -423,11 +430,11 @@ BaseCreateStack(HANDLE hProcess,
     InitialTeb->PreviousStackBase = NULL;
     InitialTeb->PreviousStackLimit = NULL;
 
-    /* Update the Stack Position */
+    /* Update the stack position */
     Stack += StackReserve - StackCommit;
 
-    /* Check if we will need a guard page */
-    if (StackReserve > StackCommit)
+    /* Check if we can add a guard page */
+    if (StackReserve >= StackCommit + PageSize)
     {
         Stack -= PageSize;
         StackCommit += PageSize;
@@ -456,11 +463,10 @@ BaseCreateStack(HANDLE hProcess,
     /* Now set the current Stack Limit */
     InitialTeb->StackLimit = (PVOID)Stack;
 
-    /* Create a guard page */
+    /* Create a guard page if needed */
     if (UseGuard)
     {
-        /* Set the guard page */
-        GuardPageSize = PAGE_SIZE;
+        GuardPageSize = PageSize;
         Status = NtProtectVirtualMemory(hProcess,
                                         (PVOID*)&Stack,
                                         &GuardPageSize,
@@ -481,10 +487,14 @@ BaseCreateStack(HANDLE hProcess,
     return STATUS_SUCCESS;
 }
 
+/*
+ * NOTE: Adapted from sdk/lib/rtl/thread.c:RtlpFreeUserStack().
+ */
 VOID
 WINAPI
-BaseFreeThreadStack(IN HANDLE hProcess,
-                    IN PINITIAL_TEB InitialTeb)
+BaseFreeThreadStack(
+    _In_ HANDLE hProcess,
+    _In_ PINITIAL_TEB InitialTeb)
 {
     SIZE_T Dummy = 0;
 
@@ -501,10 +511,10 @@ BaseFreeThreadStack(IN HANDLE hProcess,
 VOID
 WINAPI
 BaseInitializeContext(IN PCONTEXT Context,
-                       IN PVOID Parameter,
-                       IN PVOID StartAddress,
-                       IN PVOID StackAddress,
-                       IN ULONG ContextType)
+                      IN PVOID Parameter,
+                      IN PVOID StartAddress,
+                      IN PVOID StackAddress,
+                      IN ULONG ContextType)
 {
 #ifdef _M_IX86
     ULONG ContextFlags;
diff --git a/dll/win32/kernel32/include/kernel32.h
b/dll/win32/kernel32/include/kernel32.h
index 005b5d05272..ef47d4d946c 100644
--- a/dll/win32/kernel32/include/kernel32.h
+++ b/dll/win32/kernel32/include/kernel32.h
@@ -183,10 +183,17 @@ BaseFormatObjectAttributes(OUT POBJECT_ATTRIBUTES ObjectAttributes,
 
 NTSTATUS
 WINAPI
-BaseCreateStack(HANDLE hProcess,
-                 SIZE_T StackReserve,
-                 SIZE_T StackCommit,
-                 PINITIAL_TEB InitialTeb);
+BaseCreateStack(
+    _In_ HANDLE hProcess,
+    _In_opt_ SIZE_T StackCommit,
+    _In_opt_ SIZE_T StackReserve,
+    _Out_ PINITIAL_TEB InitialTeb);
+
+VOID
+WINAPI
+BaseFreeThreadStack(
+    _In_ HANDLE hProcess,
+    _In_ PINITIAL_TEB InitialTeb);
 
 VOID
 WINAPI
@@ -233,11 +240,6 @@ WINAPI
 BaseThreadStartup(LPTHREAD_START_ROUTINE lpStartAddress,
                   LPVOID lpParameter);
 
-VOID
-WINAPI
-BaseFreeThreadStack(IN HANDLE hProcess,
-                    IN PINITIAL_TEB InitialTeb);
-
 __declspec(noreturn)
 VOID
 WINAPI
diff --git a/sdk/lib/rtl/thread.c b/sdk/lib/rtl/thread.c
index 535380593c9..df7541cdeac 100644
--- a/sdk/lib/rtl/thread.c
+++ b/sdk/lib/rtl/thread.c
@@ -20,7 +20,7 @@
 
 NTSTATUS
 NTAPI
-RtlpCreateUserStack(IN HANDLE hProcess,
+RtlpCreateUserStack(IN HANDLE ProcessHandle,
                     IN SIZE_T StackReserve OPTIONAL,
                     IN SIZE_T StackCommit OPTIONAL,
                     IN ULONG StackZeroBits OPTIONAL,
@@ -29,10 +29,10 @@ RtlpCreateUserStack(IN HANDLE hProcess,
     NTSTATUS Status;
     SYSTEM_BASIC_INFORMATION SystemBasicInfo;
     PIMAGE_NT_HEADERS Headers;
-    ULONG_PTR Stack = 0;
-    BOOLEAN UseGuard = FALSE;
+    ULONG_PTR Stack;
+    BOOLEAN UseGuard;
     ULONG Dummy;
-    SIZE_T GuardPageSize;
+    SIZE_T MinimumStackCommit, GuardPageSize;
 
     /* Get some memory information */
     Status = ZwQuerySystemInformation(SystemBasicInformation,
@@ -42,26 +42,34 @@ RtlpCreateUserStack(IN HANDLE hProcess,
     if (!NT_SUCCESS(Status)) return Status;
 
     /* Use the Image Settings if we are dealing with the current Process */
-    if (hProcess == NtCurrentProcess())
+    if (ProcessHandle == NtCurrentProcess())
     {
         /* Get the Image Headers */
         Headers = RtlImageNtHeader(NtCurrentPeb()->ImageBaseAddress);
         if (!Headers) return STATUS_INVALID_IMAGE_FORMAT;
 
         /* If we didn't get the parameters, find them ourselves */
-        if (!StackReserve) StackReserve = Headers->OptionalHeader.
-                                          SizeOfStackReserve;
-        if (!StackCommit) StackCommit = Headers->OptionalHeader.
-                                        SizeOfStackCommit;
+        if (StackReserve == 0)
+            StackReserve = Headers->OptionalHeader.SizeOfStackReserve;
+        if (StackCommit == 0)
+            StackCommit = Headers->OptionalHeader.SizeOfStackCommit;
+
+        MinimumStackCommit = NtCurrentPeb()->MinimumStackCommit;
+        if ((MinimumStackCommit != 0) && (StackCommit < MinimumStackCommit))
+        {
+            StackCommit = MinimumStackCommit;
+        }
     }
     else
     {
         /* Use the System Settings if needed */
-        if (!StackReserve) StackReserve = SystemBasicInfo.AllocationGranularity;
-        if (!StackCommit) StackCommit = SystemBasicInfo.PageSize;
+        if (StackReserve == 0)
+            StackReserve = SystemBasicInfo.AllocationGranularity;
+        if (StackCommit == 0)
+            StackCommit = SystemBasicInfo.PageSize;
     }
 
-    /* Check if the commit is higher than the reserve*/
+    /* Check if the commit is higher than the reserve */
     if (StackCommit >= StackReserve)
     {
         /* Grow the reserve beyond the commit, up to 1MB alignment */
@@ -69,11 +77,12 @@ RtlpCreateUserStack(IN HANDLE hProcess,
     }
 
     /* Align everything to Page Size */
-    StackReserve = ROUND_UP(StackReserve, SystemBasicInfo.AllocationGranularity);
     StackCommit = ROUND_UP(StackCommit, SystemBasicInfo.PageSize);
+    StackReserve = ROUND_UP(StackReserve, SystemBasicInfo.AllocationGranularity);
 
     /* Reserve memory for the stack */
-    Status = ZwAllocateVirtualMemory(hProcess,
+    Stack = 0;
+    Status = ZwAllocateVirtualMemory(ProcessHandle,
                                      (PVOID*)&Stack,
                                      StackZeroBits,
                                      &StackReserve,
@@ -82,41 +91,48 @@ RtlpCreateUserStack(IN HANDLE hProcess,
     if (!NT_SUCCESS(Status)) return Status;
 
     /* Now set up some basic Initial TEB Parameters */
-    InitialTeb->PreviousStackBase = NULL;
-    InitialTeb->PreviousStackLimit = NULL;
     InitialTeb->AllocatedStackBase = (PVOID)Stack;
     InitialTeb->StackBase = (PVOID)(Stack + StackReserve);
+    InitialTeb->PreviousStackBase = NULL;
+    InitialTeb->PreviousStackLimit = NULL;
 
-    /* Update the Stack Position */
+    /* Update the stack position */
     Stack += StackReserve - StackCommit;
 
-    /* Check if we will need a guard page */
-    if (StackReserve > StackCommit)
+    /* Check if we can add a guard page */
+    if (StackReserve >= StackCommit + SystemBasicInfo.PageSize)
     {
-        /* Remove a page to set as guard page */
         Stack -= SystemBasicInfo.PageSize;
         StackCommit += SystemBasicInfo.PageSize;
         UseGuard = TRUE;
     }
+    else
+    {
+        UseGuard = FALSE;
+    }
 
     /* Allocate memory for the stack */
-    Status = ZwAllocateVirtualMemory(hProcess,
+    Status = ZwAllocateVirtualMemory(ProcessHandle,
                                      (PVOID*)&Stack,
                                      0,
                                      &StackCommit,
                                      MEM_COMMIT,
                                      PAGE_READWRITE);
-    if (!NT_SUCCESS(Status)) return Status;
+    if (!NT_SUCCESS(Status))
+    {
+        GuardPageSize = 0;
+        ZwFreeVirtualMemory(ProcessHandle, (PVOID*)&Stack, &GuardPageSize,
MEM_RELEASE);
+        return Status;
+    }
 
     /* Now set the current Stack Limit */
     InitialTeb->StackLimit = (PVOID)Stack;
 
-    /* Create a guard page */
+    /* Create a guard page if needed */
     if (UseGuard)
     {
-        /* Attempt maximum space possible */
         GuardPageSize = SystemBasicInfo.PageSize;
-        Status = ZwProtectVirtualMemory(hProcess,
+        Status = ZwProtectVirtualMemory(ProcessHandle,
                                         (PVOID*)&Stack,
                                         &GuardPageSize,
                                         PAGE_GUARD | PAGE_READWRITE,
@@ -132,23 +148,21 @@ RtlpCreateUserStack(IN HANDLE hProcess,
     return STATUS_SUCCESS;
 }
 
-NTSTATUS
+VOID
 NTAPI
-RtlpFreeUserStack(IN HANDLE Process,
+RtlpFreeUserStack(IN HANDLE ProcessHandle,
                   IN PINITIAL_TEB InitialTeb)
 {
     SIZE_T Dummy = 0;
-    NTSTATUS Status;
 
     /* Free the Stack */
-    Status = ZwFreeVirtualMemory(Process,
-                                 &InitialTeb->AllocatedStackBase,
-                                 &Dummy,
-                                 MEM_RELEASE);
+    ZwFreeVirtualMemory(ProcessHandle,
+                        &InitialTeb->AllocatedStackBase,
+                        &Dummy,
+                        MEM_RELEASE);
 
     /* Clear the initial TEB */
-    RtlZeroMemory(InitialTeb, sizeof(INITIAL_TEB));
-    return Status;
+    RtlZeroMemory(InitialTeb, sizeof(*InitialTeb));
 }
 
 /* FUNCTIONS ***************************************************************/