[PATCH v2] kernelbase: Don't pass on StdHandles if they are never to be inherited

[Brendan McGrath]

On 23/6/21 4:01 am, Jacek Caban wrote:
> On 6/22/21 8:52 AM, Brendan McGrath wrote:
>>
>> On 22/6/21 2:40 am, Jacek Caban wrote:
>>> Hi Brendan,
>>>
>>> On 6/15/21 1:17 PM, Brendan McGrath wrote:
>>>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51264
>>>> Signed-off-by: Brendan McGrath <brendan at redmandi.com>
>>>> ---
>>>> Changes since v1:
>>>> - Correct the Wine-Bug reference
>>>>
>>>>   dlls/kernelbase/process.c | 8 ++++----
>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c
>>>> index e3bd5feb296c..2833e76138bf 100644
>>>> --- a/dlls/kernelbase/process.c
>>>> +++ b/dlls/kernelbase/process.c
>>>> @@ -141,8 +141,8 @@ static WCHAR *get_file_name( WCHAR *cmdline, 
>>>> WCHAR *buffer, DWORD buflen )
>>>>    *           create_process_params
>>>>    */
>>>>   static RTL_USER_PROCESS_PARAMETERS *create_process_params( const 
>>>> WCHAR *filename, const WCHAR *cmdline,
>>>> - const WCHAR *cur_dir, void *env, DWORD flags,
>>>> - const STARTUPINFOW *startup )
>>>> + const WCHAR *cur_dir, void *env, BOOL inherit,
>>>> + DWORD flags, const STARTUPINFOW *startup )
>>>>   {
>>>>       RTL_USER_PROCESS_PARAMETERS *params;
>>>>       UNICODE_STRING imageW, curdirW, cmdlineW, titleW, desktopW, 
>>>> runtimeW, newdirW;
>>>> @@ -199,7 +199,7 @@ static RTL_USER_PROCESS_PARAMETERS 
>>>> *create_process_params( const WCHAR *filename
>>>>           params->hStdOutput = startup->hStdOutput;
>>>>           params->hStdError  = startup->hStdError;
>>>>       }
>>>> -    else if (flags & DETACHED_PROCESS)
>>>> +    else if (flags & DETACHED_PROCESS || ((flags & 
>>>> CREATE_NEW_CONSOLE) && !inherit) )
>>>
>>>
>>> Is the !inherit condition needed here? If CreateProcess (modern) 
>>> part of [1] is correct, we could probably use that branch for 
>>> CREATE_NEW_CONSOLE regardless of inherit value.
>>
>> You're right. Currently in Wine, if the inherit parameter is set to 
>> TRUE, it will inherit the StdHandles; even with CREATE_NEW_CONSOLE. 
>> But I ran a test on Windows and it did not.
>>
>>
>>> It would be great to have a test that directly uses CreateProcess() 
>>> instead of ShellExecuteEx() so that we can verify that.
>>
>> I tried adding a couple of simple tests to kernel32, but for them to 
>> work, I needed to add the '-mwindows' flag to the gcc options for 
>> kernel32_test.exe (as the tests require the child process to have the 
>> subsystem value of IMAGE_SUBSYSTEM_WINDOWS_GUI in its PE header).
>>
>> This change however causes some of the existing tests to fail on 
>> Windows:
>> https://testbot.winehq.org/JobDetails.pl?Key=92898
>>
>> The only tests that failed on the Debian machine were two of my new 
>> ones (and that's because I incorrectly marked them as todo_wine). So 
>> this shows (for the failing Windows tests) that Windows changes how 
>> it behaves based on the subsystem value in the PE header; but Wine 
>> does not. So we probably want a way to test both an 
>> IMAGE_SUBSYSTEM_WINDOWS_GUI PE subsystem value and an 
>> IMAGE_SUBSYSTEM_WINDOWS_CUI PE subsystem value (i.e. GUI vs CUI).
>>
>> But if so - what would be the best way to do that (that wouldn't 
>> impact the current testing process)? 
>
>
> Yes, we currently attach to the console too often, but in context of 
> your patch the interesting part is that when CREATE_NEW_CONSOLE is 
> used, we shouldn't try to pass handles from ProcessParameters (just 
> like we do for DETACHED_PROCESS). It indeed looks like adding such 
> test to wine tests would be more tricky than worth due to subsystem 
> requirement, 

I tried a proof of concept patch to add GUI tests to Wine:
https://testbot.winehq.org/GetFile.pl?JobKey=93007&StepKey=1

It worked on my local, but they failed to run on testbot. Possibly a 
security feature. It works by making a copy of itself, changing only the 
subsystem value - then it can be called as the child process. I've 
stopped work on this as I figure I should get your feedback on the idea 
at this stage anyway.

> but an external test would be enough. I think that modifying the test 
> that you attached to the bug would be enough. You could set a standard 
> handle to something like a pipe in the parent process, then call 
> CreateProcess() and check in the child that the handle is NULL or 
> console (but not pipe), depending on how the child is built.

I updated the tests in the bug. I tested calling GUI and CUI exes as the 
child and with inherit set to FALSE and inherit set to TRUE.

I also added output for the Teb and StartInfo structures - but the ANSI 
and Wide values were different. So I might modify the tests again to 
check what happens when I include the CREATE_UNICODE_ENVIRONMENT vs 
leaving it out.

Hopefully I'm on the right track?

>
>
> Thanks,
>
> Jacek
>