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

[Jacek Caban]

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, 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.


Thanks,

Jacek