From effdb6f232aa4448179c2b0a0f70aaa20856357c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Herm=C3=A8s=20B=C3=A9lusca-Ma=C3=AFto?= Date: Thu, 26 May 2016 04:44:25 +0200 Subject: [PATCH] [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/windowsthreadsprocessapis7_6.html - 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 ***************************************************************/ -- 2.17.1