[PATCH] kernelbase: Don't pass on StdHandles if they are never to be inherited
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51262 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- 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) ) { params->hStdInput = INVALID_HANDLE_VALUE; params->hStdOutput = INVALID_HANDLE_VALUE; @@ -545,7 +545,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalW( HANDLE token, const WCHAR info->hThread = info->hProcess = 0; info->dwProcessId = info->dwThreadId = 0; - if (!(params = create_process_params( app_name, tidy_cmdline, cur_dir, env, flags, startup_info ))) + if (!(params = create_process_params( app_name, tidy_cmdline, cur_dir, env, inherit, flags, startup_info ))) { status = STATUS_NO_MEMORY; goto done; -- 2.25.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51264 Signed-off-by: Brendan McGrath <brendan(a)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) ) { params->hStdInput = INVALID_HANDLE_VALUE; params->hStdOutput = INVALID_HANDLE_VALUE; @@ -545,7 +545,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalW( HANDLE token, const WCHAR info->hThread = info->hProcess = 0; info->dwProcessId = info->dwThreadId = 0; - if (!(params = create_process_params( app_name, tidy_cmdline, cur_dir, env, flags, startup_info ))) + if (!(params = create_process_params( app_name, tidy_cmdline, cur_dir, env, inherit, flags, startup_info ))) { status = STATUS_NO_MEMORY; goto done; -- 2.25.1
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(a)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. It would be great to have a test that directly uses CreateProcess() instead of ShellExecuteEx() so that we can verify that. Thanks, Jacek [1] https://github.com/rprichard/win32-console-docs/blob/master/README.md
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(a)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)?
Thanks,
Jacek
[1] https://github.com/rprichard/win32-console-docs/blob/master/README.md
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(a)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
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(a)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
On 6/24/21 5:10 AM, Brendan McGrath wrote:
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?
Yes, that's what I was looking for, thanks. I think it proves that we we should never pass parent std handles when CREATE_NEW_CONSOLE is used, regardless of inherit flag. In that case, your original patch modified to not check inherit would look good to me. Thanks, Jacek
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51264 Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- Changes since v2: - no longer checking the inherit flag (as it shouldn't be passed on either way) dlls/kernelbase/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 924fa733e5b5..7296e0853826 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -198,7 +198,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 | CREATE_NEW_CONSOLE)) { params->hStdInput = INVALID_HANDLE_VALUE; params->hStdOutput = INVALID_HANDLE_VALUE; -- 2.25.1
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>
participants (2)
-
Brendan McGrath -
Jacek Caban