[PATCH 1/2] ntdll/tests: Check that creating huge thread stacks should work.

[Gabriel Ivăncescu]

On 07/05/2021 14:58, Rémi Bernon wrote:
> On 5/7/21 1:56 PM, Gabriel Ivăncescu wrote:
>> On 07/05/2021 14:44, Rémi Bernon wrote:
>>> On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
>>>> On 07/05/2021 13:20, Rémi Bernon wrote:
>>>>> On 5/7/21 12:08 PM, Rémi Bernon wrote:
>>>>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>>>>> ---
>>>>>>
>>>>>> I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
>>>>>> was for, but it seems to be causing spurious failures. This fix is
>>>>>> trying to keep the current behavior, but it may be better to 
>>>>>> completely
>>>>>> revert the commit as, although less likely, TEB allocation could also
>>>>>> fail much more often.
>>>>>>
>>>>>>   dlls/ntdll/tests/virtual.c | 10 ++++++++++
>>>>>>   1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
>>>>>> index 8f5b0092bea..686b4076801 100644
>>>>>> --- a/dlls/ntdll/tests/virtual.c
>>>>>> +++ b/dlls/ntdll/tests/virtual.c
>>>>>> @@ -488,6 +488,11 @@ static DWORD WINAPI 
>>>>>> test_stack_size_thread(void *ptr)
>>>>>>       return 0;
>>>>>>   }
>>>>>> +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static void test_RtlCreateUserStack(void)
>>>>>>   {
>>>>>>       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
>>>>>> NtCurrentTeb()->Peb->ImageBaseAddress );
>>>>>> @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
>>>>>>       thread = CreateThread(NULL, 0x3ff000, 
>>>>>> test_stack_size_thread, &args, STACK_SIZE_PARAM_IS_A_RESERVATION, 
>>>>>> NULL);
>>>>>>       WaitForSingleObject(thread, INFINITE);
>>>>>>       CloseHandle(thread);
>>>>>> +
>>>>>> +    thread = CreateThread(NULL, 0x80000000, 
>>>>>> test_stack_size_dummy_thread, NULL, 
>>>>>> STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
>>>>>> +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
>>>>>> failed\n");
>>>>>> +    WaitForSingleObject(thread, INFINITE);
>>>>>> +    CloseHandle(thread);
>>>>>>   }
>>>>>>   static void test_NtMapViewOfSection(void)
>>>>>>
>>>>>
>>>>> Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
>>>>> the fix, it's maybe something related to allocations that should be 
>>>>> bottom-up by default?
>>>>
>>>> There's a wine-staging patch that does this and fixes some bugs:
>>>>
>>>> ntdll-ForceBottomUpAlloc
>>>>
>>>> Does it help? Also what happens on Windows if you force top-down 
>>>> allocation?
>>>
>>> It doesn't help with the regression, because we now forcefully limit 
>>> the address space where the thread stack / teb can be allocated.
>>>
>>> What I meant was that although I don't know why it's been added, and 
>>> if the reason was to make sure early threads are allocated on low 
>>> addresses, maybe making default allocation strategy bottom-up is the 
>>> correct fix (instead of trying to forcefully put them in the low 2G).
>>>
>>> I think that patch set has been sent already before but it was 
>>> considered a bit over complicated.
>>
>> Yeah, but if all allocations are forced bottom-up, not just the thread 
>> stack/TEB, then the staging patch should already fix this, implicitly.
>>
> 
> I think it makes things ever worse as more things will be allocated in 
> the low address space?
> 

AFAIK that's already the case on Windows (bottom-up default allocation 
behavior), and some things are known to break if top-down is forced in 
the registry. Even Microsoft's own xaudio breaks. Not related to stack 
or teb.

I don't know the purpose of the current code, I'm just assuming it might 
be a semi-hack (probably dealing with apps that expect stack in lower 
2G) and the "correct" fix would be to force bottom up allocation, as 
that also fixes other apps and matches Windows default behavior. Then we 
won't need this. And of course, I doubt Windows restricts the stack to 
low 2G if it doesn't have any more room there. Anyway this will need 
investigation.