[PATCH 0/2] MR9500: In CreateProcess(), set std handle when passing a pseudo-console.
This MR tests and fix the std handles in child process when using CreateProcess with extended information and passing a pseudo-console handle.
From the added tests, and other manual testing (see linked bugzilla entry), it turns out native overrides the (bound) std handles from the parent console with handles on the passed pseudo-console for the child process. But happily inherits other kind of handles.
As side notes: - resetting the std handle in RTL_USER_PROCESS_PARAMETERS being console handles could be optimized: potentially, native could just test if the passed handles are bound to the passed console and reset them if not. - hijacked an unused bit in ConsoleFlags to discriminate when to recreate the std handles in child (didn't check-out how native does it though). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9500
From: Eric Pouech <epouech(a)codeweavers.com> Partial fix for Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58967 Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/process.c | 110 +++++++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 8 deletions(-) diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 948b9bfd709..a503f763009 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -3420,6 +3420,42 @@ static BOOL build_startupinfo( STARTUPINFOA *startup, unsigned args, HANDLE hstd return needs_close; } +static BOOL build_startupinfoex( STARTUPINFOEXA *startup, unsigned args, HANDLE hstd[4], HPCON *pseudo_console ) +{ + HANDLE outPipeOurSide, inPipeOurSide; + HANDLE outPipePseudoConsoleSide, inPipePseudoConsoleSide; + SIZE_T attr_size; + COORD size = {80, 32}; + HRESULT hres; + BOOL ret, needs_close; + + ret = CreatePipe(&inPipePseudoConsoleSide, &inPipeOurSide, NULL, 0); + ok(ret, "Couldn't create pipe\n"); + ret = CreatePipe(&outPipeOurSide, &outPipePseudoConsoleSide, NULL, 0); + ok(ret, "Couldn't create pipe\n"); + + hres = CreatePseudoConsole(size, inPipePseudoConsoleSide, outPipePseudoConsoleSide, 0, pseudo_console); + ok(hres == S_OK, "CreatePseudoConsole failed: %08lx\n", hres); + + memset(startup, 0, sizeof(*startup)); + needs_close = build_startupinfo(&startup->StartupInfo, args, hstd); + startup->StartupInfo.cb = sizeof(*startup); + + InitializeProcThreadAttributeList(NULL, 1, 0, &attr_size); + startup->lpAttributeList = HeapAlloc(GetProcessHeap(), 0, attr_size); + InitializeProcThreadAttributeList(startup->lpAttributeList, 1, 0, &attr_size); + UpdateProcThreadAttribute(startup->lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, *pseudo_console, + sizeof(*pseudo_console), NULL, NULL); + + hstd[2] = outPipeOurSide; + hstd[3] = inPipeOurSide; + + CloseHandle(outPipePseudoConsoleSide); + CloseHandle(inPipePseudoConsoleSide); + + return needs_close; +} + struct std_handle_test { /* input */ @@ -3515,6 +3551,41 @@ static void test_StdHandleInheritance(void) /* all others handles type behave as H_DISK */ {ARG_STD | ARG_CP_INHERIT | ARG_HANDLE_INHERIT | H_DISK, HATTR_NULL}, {ARG_STARTUPINFO | ARG_CP_INHERIT | ARG_HANDLE_INHERIT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_DISK}, + }, + pseudoconsole_cui[] = + { + /* all others handles type behave as H_DISK */ + {ARG_STARTUPINFO | ARG_CP_INHERIT | ARG_HANDLE_INHERIT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_DISK}, + {ARG_STD | ARG_CP_INHERIT | ARG_HANDLE_INHERIT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_DISK}, + + /* all others handles type behave as H_DISK */ + {ARG_STARTUPINFO | ARG_HANDLE_INHERIT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | ARG_HANDLE_INHERIT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_DISK}, + + /* all others handles type behave as H_DISK (except H_CONSOLE) */ + {ARG_STARTUPINFO | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | H_DISK, HATTR_TYPE | FILE_TYPE_DISK}, + + /* all others handles type behave as H_DISK (except H_CONSOLE) */ + {ARG_STARTUPINFO | ARG_HANDLE_PROTECT | H_DISK, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | ARG_HANDLE_PROTECT | H_DISK, HATTR_TYPE | HATTR_PROTECT | FILE_TYPE_DISK}, + + /* all others handles type behave as H_DISK */ + {ARG_STARTUPINFO | ARG_CP_INHERIT | H_DISK, HATTR_DANGLING, .is_broken = HATTR_TYPE | FILE_TYPE_UNKNOWN}, + {ARG_STD | ARG_CP_INHERIT | H_DISK, HATTR_DANGLING}, + + {ARG_STARTUPINFO | H_DEVIL, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | H_DEVIL, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STARTUPINFO | H_INVALID, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | H_INVALID, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STARTUPINFO | H_NULL, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | H_NULL, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + + /* looks like parent handle is not inherited */ + {ARG_STARTUPINFO | H_CONSOLE, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | H_CONSOLE, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STARTUPINFO | ARG_HANDLE_PROTECT | H_CONSOLE, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, + {ARG_STD | ARG_HANDLE_PROTECT | H_CONSOLE, HATTR_TYPE | HATTR_INHERIT | FILE_TYPE_CHAR}, }; static const struct { @@ -3527,10 +3598,13 @@ static void test_StdHandleInheritance(void) tests[] = { #define X(d, cg, s) {(d), (cg), s, ARRAY_SIZE(s), #s} - X(0, TRUE, nothing_cui), - X(0, FALSE, nothing_gui), - X(DETACHED_PROCESS, TRUE, detached_cui), - X(DETACHED_PROCESS, FALSE, detached_gui), + X(0, TRUE, nothing_cui), + X(0, FALSE, nothing_gui), + X(DETACHED_PROCESS, TRUE, detached_cui), + X(DETACHED_PROCESS, FALSE, detached_gui), + X(EXTENDED_STARTUPINFO_PRESENT, TRUE, pseudoconsole_cui), + X(EXTENDED_STARTUPINFO_PRESENT | DETACHED_PROCESS, TRUE, detached_cui), + /* not testing gui + pseudo console, behaves like without pseudo console */ #undef X }; @@ -3554,16 +3628,20 @@ static void test_StdHandleInheritance(void) for (i = 0; i < tests[j].count; i++) { - STARTUPINFOA startup; - HANDLE hstd[2] = {}; + STARTUPINFOEXA startup; + HPCON pseudo_console; + HANDLE hstd[4] = {}; BOOL needs_close; winetest_push_context("%s[%u] ", tests[j].descr, i); - needs_close = build_startupinfo( &startup, std_tests[i].args, hstd ); + if (tests[j].cp_flags & EXTENDED_STARTUPINFO_PRESENT) + needs_close = build_startupinfoex( &startup, std_tests[i].args, hstd, &pseudo_console ); + else + needs_close = build_startupinfo( &startup.StartupInfo, std_tests[i].args, hstd ); ret = check_run_child(tests[j].use_cui ? cuiexec : guiexec, tests[j].cp_flags, !!(std_tests[i].args & ARG_CP_INHERIT), - &startup); + &startup.StartupInfo); ok(ret, "Couldn't run child\n"); reload_child_info(resfile); @@ -3591,16 +3669,25 @@ static void test_StdHandleInheritance(void) { unsigned startup_expected = (std_tests[i].args & ARG_STD) ? HATTR_INVALID : std_tests[i].expected; + todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || i == 10 || i == 12 || i == 14 || i == 16 || i == 18)) + { okChildHexInt("StartupInfoA", "hStdInputEncode", startup_expected, std_tests[i].is_broken); okChildHexInt("StartupInfoA", "hStdOutputEncode", startup_expected, std_tests[i].is_broken); + } startup_expected = (std_tests[i].args & ARG_STD) ? HATTR_UNTOUCHED : std_tests[i].expected; + todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || i == 10 || i == 12 || i == 14 || i == 16 || i == 18)) + { okChildHexInt("StartupInfoW", "hStdInputEncode", startup_expected, std_tests[i].is_broken); okChildHexInt("StartupInfoW", "hStdOutputEncode", startup_expected, std_tests[i].is_broken); + } + todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || (i >= 10 && i <= 19))) + { okChildHexInt("TEB", "hStdInputEncode", std_tests[i].expected, std_tests[i].is_broken); okChildHexInt("TEB", "hStdOutputEncode", std_tests[i].expected, std_tests[i].is_broken); + } } release_memory(); @@ -3612,6 +3699,13 @@ static void test_StdHandleInheritance(void) SetHandleInformation(hstd[1], HANDLE_FLAG_PROTECT_FROM_CLOSE, 0); CloseHandle(hstd[1]); } + if (tests[j].cp_flags & EXTENDED_STARTUPINFO_PRESENT) + { + HeapFree(GetProcessHeap(), 0, startup.lpAttributeList); + ClosePseudoConsole(pseudo_console); + CloseHandle(hstd[2]); + CloseHandle(hstd[3]); + } winetest_pop_context(); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9500
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/process.c | 9 --------- dlls/kernelbase/console.c | 7 +++++++ dlls/kernelbase/process.c | 12 ++++++++++++ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index a503f763009..81883e64b63 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -3669,25 +3669,16 @@ static void test_StdHandleInheritance(void) { unsigned startup_expected = (std_tests[i].args & ARG_STD) ? HATTR_INVALID : std_tests[i].expected; - todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || i == 10 || i == 12 || i == 14 || i == 16 || i == 18)) - { okChildHexInt("StartupInfoA", "hStdInputEncode", startup_expected, std_tests[i].is_broken); okChildHexInt("StartupInfoA", "hStdOutputEncode", startup_expected, std_tests[i].is_broken); - } startup_expected = (std_tests[i].args & ARG_STD) ? HATTR_UNTOUCHED : std_tests[i].expected; - todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || i == 10 || i == 12 || i == 14 || i == 16 || i == 18)) - { okChildHexInt("StartupInfoW", "hStdInputEncode", startup_expected, std_tests[i].is_broken); okChildHexInt("StartupInfoW", "hStdOutputEncode", startup_expected, std_tests[i].is_broken); - } - todo_wine_if(j == 4 && (i == 2 || i == 4 || i == 6 || (i >= 10 && i <= 19))) - { okChildHexInt("TEB", "hStdInputEncode", std_tests[i].expected, std_tests[i].is_broken); okChildHexInt("TEB", "hStdOutputEncode", std_tests[i].expected, std_tests[i].is_broken); - } } release_memory(); diff --git a/dlls/kernelbase/console.c b/dlls/kernelbase/console.c index 83bec6c9878..4a0f96076b5 100644 --- a/dlls/kernelbase/console.c +++ b/dlls/kernelbase/console.c @@ -2390,5 +2390,12 @@ void init_console( void ) alloc_console( no_window ); } else if (params->ConsoleHandle && params->ConsoleHandle != CONSOLE_HANDLE_SHELL_NO_WINDOW) + { create_console_connection( params->ConsoleHandle ); + if (params->ConsoleFlags & 2) + { + init_console_std_handles( FALSE ); + params->ConsoleFlags &= ~2; + } + } } diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index d89b2f1405a..92c9fd511d8 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -606,6 +606,18 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalW( HANDLE token, const WCHAR TRACE( "PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE %p reference %p\n", console, console->reference ); params->ConsoleHandle = console->reference; + if (!(flags & DETACHED_PROCESS)) + { + params->ConsoleFlags |= 2; + /* don't inherit standard handles bound to parent console (but inherit the others) */ + if (!(startup_info->dwFlags & STARTF_USESTDHANDLES)) + { + DWORD mode; + if (GetConsoleMode(params->hStdInput, &mode)) params->hStdInput = NULL; + if (GetConsoleMode(params->hStdOutput, &mode)) params->hStdOutput = NULL; + if (GetConsoleMode(params->hStdError, &mode)) params->hStdError = NULL; + } + } break; } case PROC_THREAD_ATTRIBUTE_JOB_LIST: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9500
Jacek Caban (@jacek) commented about dlls/kernel32/tests/process.c:
return needs_close; }
+static BOOL build_startupinfoex( STARTUPINFOEXA *startup, unsigned args, HANDLE hstd[4], HPCON *pseudo_console ) +{ + HANDLE outPipeOurSide, inPipeOurSide; + HANDLE outPipePseudoConsoleSide, inPipePseudoConsoleSide;
Could you use more consistent names here? Also, Wine-Bug: would fit better in the other commit. Looks good to me overall. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9500#note_122576
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Jacek Caban (＠jacek)