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@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