[PATCH v3 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).
-- v3: kernelbase: Create std handles from passed pseudo-console. kernel32/tests: Test std handles in CreateProcess with pseudo console.
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@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..51733bc5f5d 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 output_read, input_write; + HANDLE output_write, input_read; + SIZE_T attr_size; + COORD size = {80, 32}; + HRESULT hres; + BOOL ret, needs_close; + + ret = CreatePipe(&input_read, &input_write, NULL, 0); + ok(ret, "Couldn't create pipe\n"); + ret = CreatePipe(&output_read, &output_write, NULL, 0); + ok(ret, "Couldn't create pipe\n"); + + hres = CreatePseudoConsole(size, input_read, output_write, 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] = output_read; + hstd[3] = input_write; + + CloseHandle(output_write); + CloseHandle(input_read); + + 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(); } }
From: Eric Pouech epouech@codeweavers.com
Partial fix for Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58967
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/kernel32/tests/process.c | 9 --------- dlls/kernelbase/console.c | 16 ++++++++++++++++ dlls/kernelbase/kernelbase.h | 1 + dlls/kernelbase/process.c | 11 +++++++++++ 4 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 51733bc5f5d..56b152a7f31 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..2b7f7ba5146 100644 --- a/dlls/kernelbase/console.c +++ b/dlls/kernelbase/console.c @@ -111,6 +111,15 @@ static BOOL console_ioctl( HANDLE handle, DWORD code, void *in_buff, DWORD in_co return set_ntstatus( status ); }
+BOOL is_console_handle( HANDLE handle ) +{ + IO_STATUS_BLOCK io; + DWORD mode; + + return NtDeviceIoControlFile( handle, NULL, NULL, NULL, &io, IOCTL_CONDRV_GET_MODE, + NULL, 0, &mode, sizeof(mode) ) == STATUS_SUCCESS; +} + /* map input records to ASCII */ static void input_records_WtoA( INPUT_RECORD *buffer, int count ) { @@ -2390,5 +2399,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/kernelbase.h b/dlls/kernelbase/kernelbase.h index 2209939a8a2..e46ca0f6257 100644 --- a/dlls/kernelbase/kernelbase.h +++ b/dlls/kernelbase/kernelbase.h @@ -37,6 +37,7 @@ extern void init_global_data(void); extern void init_startup_info( RTL_USER_PROCESS_PARAMETERS *params ); extern void init_locale( HMODULE module ); extern void init_console(void); +extern BOOL is_console_handle( HANDLE );
extern const WCHAR windows_dir[]; extern const WCHAR system_dir[]; diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index d89b2f1405a..3656e40280d 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -606,6 +606,17 @@ 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)) + { + if (is_console_handle(params->hStdInput)) params->hStdInput = NULL; + if (is_console_handle(params->hStdOutput)) params->hStdOutput = NULL; + if (is_console_handle(params->hStdError)) params->hStdError = NULL; + } + } break; } case PROC_THREAD_ATTRIBUTE_JOB_LIST:
Pushed V3:
* didn't add the test for not tempering last error, as there are (at least) 5 sites in CreateProcessInternalW which call other kernelbase API which temper last error; fixing all these would have been a too large task (and should wait for an app to actually need it) * using helper to test console handle (at least we're not adding a 6th call site)
This merge request was approved by Jacek Caban.
-
Eric Pouech -
eric pouech (＠epo)
-
Jacek Caban (＠jacek)