[PATCH v4 0/3] MR3312: programs/start: Allow Ctrl-C to terminate GUI applications (/exec mode).
Since dcf0bf1f, one could no longer kill a GUI application launched from unix shell (with start.exe /exec) with ctrl-c. (depends on https://gitlab.winehq.org/wine/wine/-/merge_requests/3310 to be fully functional). Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> -- v4: programs/cmd: Better handle ctrl-c events. ntdll,server: Terminate GUI programs launched from unix shell. kernel32/console: Add tests for GenerateConsoleCtrlEvent(). https://gitlab.winehq.org/wine/wine/-/merge_requests/3312
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/console.c | 152 ++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index 6facf2ad214..54d1a80709b 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -5153,6 +5153,142 @@ static void test_CreateProcessCUI(void) SetStdHandle(STD_ERROR_HANDLE, hstd[2]); } +#define NO_EVENT 0xfe + +static HANDLE mch_child_kill_event; +static DWORD mch_child_event = NO_EVENT; +static BOOL WINAPI mch_child(DWORD event) +{ + mch_child_event = event; + SetEvent(mch_child_kill_event); + return TRUE; +} + +static void test_CtrlHandlerSubsystem(void) +{ + static char guiexec[MAX_PATH]; + static char cuiexec[MAX_PATH]; + + static struct + { + /* input */ + BOOL use_cui; + DWORD cp_flags; + enum pgid {PGID_PARENT, PGID_ZERO, PGID_CHILD} pgid_kind; + /* output */ + unsigned child_event; + } + tests[] = + { +/* 0 */ {FALSE, 0, PGID_PARENT, NO_EVENT}, + {FALSE, 0, PGID_ZERO, NO_EVENT}, + {FALSE, CREATE_NEW_PROCESS_GROUP, PGID_CHILD, NO_EVENT}, + {FALSE, CREATE_NEW_PROCESS_GROUP, PGID_PARENT, NO_EVENT}, + {FALSE, CREATE_NEW_PROCESS_GROUP, PGID_ZERO, NO_EVENT}, +/* 5 */ {TRUE, 0, PGID_PARENT, CTRL_C_EVENT}, + {TRUE, 0, PGID_ZERO, CTRL_C_EVENT}, + {TRUE, CREATE_NEW_PROCESS_GROUP, PGID_CHILD, NO_EVENT}, + {TRUE, CREATE_NEW_PROCESS_GROUP, PGID_PARENT, NO_EVENT}, + {TRUE, CREATE_NEW_PROCESS_GROUP, PGID_ZERO, NO_EVENT}, +/* 10 */ {TRUE, CREATE_NEW_CONSOLE, PGID_PARENT, NO_EVENT}, + {TRUE, CREATE_NEW_CONSOLE, PGID_ZERO, NO_EVENT}, + {TRUE, DETACHED_PROCESS, PGID_PARENT, NO_EVENT}, + {TRUE, DETACHED_PROCESS, PGID_ZERO, NO_EVENT}, + }; + SECURITY_ATTRIBUTES inheritable_attr = { sizeof(inheritable_attr), NULL, TRUE }; + STARTUPINFOA si = { sizeof(si) }; + PROCESS_INFORMATION info; + char buf[MAX_PATH]; + DWORD exit_code; + HANDLE event_child; + char **argv; + DWORD saved_console_flags; + DWORD pgid; + BOOL ret; + DWORD res; + int i; + + winetest_get_mainargs(&argv); + GetTempPathA(ARRAY_SIZE(guiexec), guiexec); + strcat(guiexec, "console_gui.exe"); + copy_change_subsystem(argv[0], guiexec, IMAGE_SUBSYSTEM_WINDOWS_GUI); + GetTempPathA(ARRAY_SIZE(cuiexec), cuiexec); + strcat(cuiexec, "console_cui.exe"); + copy_change_subsystem(argv[0], cuiexec, IMAGE_SUBSYSTEM_WINDOWS_CUI); + + event_child = CreateEventA(&inheritable_attr, FALSE, FALSE, NULL); + ok(event_child != NULL, "Couldn't create event\n"); + + saved_console_flags = RtlGetCurrentPeb()->ProcessParameters->ConsoleFlags; + + /* protect self against ctrl-c, but don't mask it on child */ + ret = SetConsoleCtrlHandler(NULL, FALSE); + ret = SetConsoleCtrlHandler(mydummych, TRUE); + ok(ret, "Couldn't set ctrl-c handler flag\n"); + + for (i = 0; i < ARRAY_SIZE(tests); i++) + { + winetest_push_context("test #%u", i); + + res = snprintf(buf, ARRAY_SIZE(buf), "\"%s\" console ctrl_handler %p", tests[i].use_cui ? cuiexec : guiexec, event_child); + ok((LONG)res >= 0 && res < ARRAY_SIZE(buf), "Truncated string %s (%lu)\n", buf, res); + + ret = CreateProcessA(NULL, buf, NULL, NULL, TRUE, tests[i].cp_flags, + NULL, NULL, &si, &info); + ok(ret, "CreateProcess failed: %lu %s\n", GetLastError(), tests[i].use_cui ? cuiexec : guiexec); + + res = WaitForSingleObject(event_child, 5000); + ok(res == WAIT_OBJECT_0, "Child didn't init %lu %p\n", res, event_child); + + switch (tests[i].pgid_kind) + { + case PGID_PARENT: + pgid = RtlGetCurrentPeb()->ProcessParameters->ProcessGroupId; + break; + case PGID_CHILD: + ok((tests[i].cp_flags & CREATE_NEW_PROCESS_GROUP) != 0, + "PGID should only be used with new process groupw\n"); + pgid = info.dwProcessId; + break; + case PGID_ZERO: + pgid = 0; + break; + default: + ok(0, "Unexpected pgid kind %u\n", tests[i].pgid_kind); + pgid = 0; + } + + ret = GenerateConsoleCtrlEvent(CTRL_C_EVENT, pgid); + ok(ret || broken(GetLastError() == ERROR_INVALID_PARAMETER) /* Win7 */, + "GenerateConsoleCtrlEvent failed: %lu\n", GetLastError()); + + res = WaitForSingleObject(info.hProcess, 2000); + ok(res == WAIT_OBJECT_0, "Expecting child to be terminated\n"); + + if (ret) + { + ret = GetExitCodeProcess(info.hProcess, &exit_code); + ok(ret, "Couldn't get exit code\n"); + + ok(tests[i].child_event == exit_code, "Unexpected exit code %#lx, instead of %#x\n", + exit_code, tests[i].child_event); + } + + CloseHandle(info.hProcess); + CloseHandle(info.hThread); + winetest_pop_context(); + } + + CloseHandle(event_child); + + RtlGetCurrentPeb()->ProcessParameters->ConsoleFlags = saved_console_flags; + ret = SetConsoleCtrlHandler(mydummych, FALSE); + ok(ret, "Couldn't remove ctrl-c handler flag\n"); + + DeleteFileA(guiexec); + DeleteFileA(cuiexec); +} + START_TEST(console) { HANDLE hConIn, hConOut, revert_output = NULL, unbound_output; @@ -5181,6 +5317,21 @@ START_TEST(console) return; } + if (argc == 4 && !strcmp(argv[2], "ctrl_handler")) + { + HANDLE event; + + SetConsoleCtrlHandler(mch_child, TRUE); + mch_child_kill_event = CreateEventA(NULL, FALSE, FALSE, NULL); + ok(mch_child_kill_event != NULL, "Couldn't create event\n"); + sscanf(argv[3], "%p", &event); + ret = SetEvent(event); + ok(ret, "SetEvent failed\n"); + + WaitForSingleObject(mch_child_kill_event, 1000); /* enough for all events to be distributed? */ + ExitProcess(mch_child_event); + } + if (argc == 3 && !strcmp(argv[2], "check_console")) { DWORD exit_code = 0, pcslist; @@ -5408,6 +5559,7 @@ START_TEST(console) test_AllocConsole(); test_FreeConsole(); test_CreateProcessCUI(); + test_CtrlHandlerSubsystem(); } else if (revert_output) SetConsoleActiveScreenBuffer(revert_output); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3312
From: Eric Pouech <epouech(a)codeweavers.com> Since dcf0bf1f, one could no longer kill a GUI application launched from unix shell (with start.exe /exec) with ctrl-c. Basically, when a GUI program is run from a unix shell, we must propagate ctrl-c using the unix process group. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 +- include/wine/condrv.h | 5 +++-- programs/conhost/conhost.c | 2 +- server/console.c | 35 ++++++++++++++++++++--------------- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index fc2d2ff7a12..29297202784 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3219,7 +3219,7 @@ NTSTATUS WINAPI __wine_ctrl_routine( void *arg ) { DWORD ret = 0; - if (pCtrlRoutine && NtCurrentTeb()->Peb->ProcessParameters->ConsoleHandle) ret = pCtrlRoutine( arg ); + if (pCtrlRoutine) ret = pCtrlRoutine( arg ); RtlExitUserThread( ret ); } diff --git a/include/wine/condrv.h b/include/wine/condrv.h index 51534720444..1a5c4f4555b 100644 --- a/include/wine/condrv.h +++ b/include/wine/condrv.h @@ -190,8 +190,9 @@ struct condrv_scroll_params /* IOCTL_CONDRV_CTRL_EVENT params */ struct condrv_ctrl_event { - int event; /* the event to send */ - unsigned int group_id; /* the group to send the event to */ + int event; /* the event to send */ + unsigned int group_id; /* the group to send the event to */ + /* (when from server, target process group: 0 = windows, 1 = unix) */ }; /* Wine specific values for console inheritance (params->ConsoleHandle) */ diff --git a/programs/conhost/conhost.c b/programs/conhost/conhost.c index eb3c574789f..38cfb16dbdb 100644 --- a/programs/conhost/conhost.c +++ b/programs/conhost/conhost.c @@ -1518,7 +1518,7 @@ NTSTATUS write_console_input( struct console *console, const INPUT_RECORD *recor IO_STATUS_BLOCK io; ctrl_event.event = event; - ctrl_event.group_id = 0; + ctrl_event.group_id = console->is_unix; NtDeviceIoControlFile( console->server, NULL, NULL, NULL, &io, IOCTL_CONDRV_CTRL_EVENT, &ctrl_event, sizeof(ctrl_event), NULL, 0 ); } diff --git a/server/console.c b/server/console.c index b64283baf4a..87ea6ec1808 100644 --- a/server/console.c +++ b/server/console.c @@ -663,16 +663,22 @@ struct thread *console_get_renderer( struct console *console ) struct console_signal_info { - struct console *console; - process_id_t group; - int signal; + struct console *console; + process_id_t group; + int unix_pgid; + int signal; }; static int propagate_console_signal_cb(struct process *process, void *user) { struct console_signal_info* csi = (struct console_signal_info*)user; + unsigned cmp; - if (process->console == csi->console && (!csi->group || process->group_id == csi->group)) + if (csi->unix_pgid) + cmp = csi->unix_pgid == getpgid(process->unix_pid) && csi->console->renderer->process != process; + else + cmp = process->console == csi->console && (!csi->group || process->group_id == csi->group); + if (cmp) { /* find a suitable thread to signal */ struct thread *thread; @@ -684,8 +690,8 @@ static int propagate_console_signal_cb(struct process *process, void *user) return FALSE; } -static void propagate_console_signal( struct console *console, - int sig, process_id_t group_id ) +static void propagate_console_signal( int sig, struct console *console, + process_id_t group_id, int unix_pgid ) { struct console_signal_info csi; @@ -703,8 +709,9 @@ static void propagate_console_signal( struct console *console, set_error( STATUS_NOT_IMPLEMENTED ); return; } - csi.console = console; - csi.group = group_id; + csi.console = console; + csi.group = group_id; + csi.unix_pgid = unix_pgid; enum_processes(propagate_console_signal_cb, &csi); } @@ -983,7 +990,7 @@ static void console_ioctl( struct fd *fd, ioctl_code_t code, struct async *async set_error( STATUS_INVALID_PARAMETER ); return; } - propagate_console_signal( console, event->event, group ); + propagate_console_signal( event->event, console, group, 0 ); return; } @@ -1141,12 +1148,10 @@ static void console_server_ioctl( struct fd *fd, ioctl_code_t code, struct async set_error( STATUS_INVALID_PARAMETER ); return; } - if (!server->console) - { - set_error( STATUS_INVALID_HANDLE ); - return; - } - propagate_console_signal( server->console, event->event, event->group_id ); + if (event->group_id) + propagate_console_signal( event->event, server->console, 0, getpgid(server->console->renderer->process->unix_pid)); + else + propagate_console_signal( event->event, server->console, 0, 0); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3312
From: Eric Pouech <epouech(a)codeweavers.com> cmd shouldn't terminate itself when user hits ctrl-c. cmd should terminate the currently running CUI child it waits for upon ctrl-c. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- programs/cmd/wcmdmain.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index d00c6e07566..cc81534fff3 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -2411,6 +2411,12 @@ void WCMD_free_commands(CMD_LIST *cmds) { } } +static BOOL WINAPI my_event_handler(DWORD ctrl) +{ + WCMD_output(L"\n"); + return ctrl == CTRL_C_EVENT; +} + /***************************************************************************** * Main entry point. This is a console application so we have a main() not a @@ -2658,6 +2664,10 @@ int __cdecl wmain (int argc, WCHAR *argvW[]) if (opt_s && *cmd=='\"') WCMD_strip_quotes(cmd); } + else + { + SetConsoleCtrlHandler(my_event_handler, TRUE); + } /* Save cwd into appropriate env var (Must be before the /c processing */ GetCurrentDirectoryW(ARRAY_SIZE(string), string); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3312
On Tue Aug 29 10:10:00 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136749 Your paranoid android. === w7u_2qxl (32 bit report) === kernel32: console.c:5260: Test failed: test #0: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #3: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #5: GenerateConsoleCtrlEvent failed: 87 console.c:5267: Test failed: test #5: Unexpected exit code 0xfe, instead of 0 console.c:5260: Test failed: test #8: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #10: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #12: GenerateConsoleCtrlEvent failed: 87 === w7u_adm (32 bit report) === kernel32: console.c:5260: Test failed: test #0: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #3: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #5: GenerateConsoleCtrlEvent failed: 87 console.c:5267: Test failed: test #5: Unexpected exit code 0xfe, instead of 0 console.c:5260: Test failed: test #8: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #10: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #12: GenerateConsoleCtrlEvent failed: 87 === w7u_el (32 bit report) === kernel32: console.c:5260: Test failed: test #0: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #3: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #5: GenerateConsoleCtrlEvent failed: 87 console.c:5267: Test failed: test #5: Unexpected exit code 0xfe, instead of 0 console.c:5260: Test failed: test #8: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #10: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #12: GenerateConsoleCtrlEvent failed: 87 === w1064v1507 (32 bit report) === kernel32: console.c:5239: Test failed: test #4: Child didn't init 258 00000300 console.c:5267: Test failed: test #5: Unexpected exit code 0xfe, instead of 0 console.c:5267: Test failed: test #6: Unexpected exit code 0xfe, instead of 0 console.c:5267: Test failed: test #7: Unexpected exit code 0xc0000142, instead of 0xfe === w7pro64 (64 bit report) === kernel32: console.c:5260: Test failed: test #0: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #3: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #5: GenerateConsoleCtrlEvent failed: 87 console.c:5267: Test failed: test #5: Unexpected exit code 0xfe, instead of 0 console.c:5260: Test failed: test #8: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #10: GenerateConsoleCtrlEvent failed: 87 console.c:5260: Test failed: test #12: GenerateConsoleCtrlEvent failed: 87 === w1064v1507 (64 bit report) === kernel32: console.c:5239: Test failed: test #3: Child didn't init 258 0000000000000114 console.c:5263: Test failed: test #5: Expecting child to be terminated console.c:5267: Test failed: test #5: Unexpected exit code 0x103, instead of 0 console.c:5267: Test failed: test #6: Unexpected exit code 0xfe, instead of 0 console.c:5267: Test failed: test #7: Unexpected exit code 0xc0000142, instead of 0xfe === w1064_adm (64 bit report) === kernel32: console.c:5239: Test failed: test #11: Child didn't init 258 0000000000000210 ``` V3 shall fix these failures
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3312#note_43664
Do we really need all this special-casing for unix consoles? It seems to me that with !3442, we could entirely depend on Unix to propagate signals and the change to `__wine_ctrl_routine` should be enough to fix the bug. I guess you want to propagate the signal in case an application does any read console as in this case the application does read console, like launching cmd from Unix console and then notepad from there. However, it's not obvious to me if we still want to propagate signals and even less sure if it's worth all the extra special-casing. Giving cmd more control and not terminating its child processes behind its back is more compatible with Windows in this case and does not seem wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3312#note_44318
yes that was I was targetting (note that cmd does read input, so it's not the actual use-case, but more a CUI program behaving like "start /exec" but without the termination of child if parent terminates) what could be misleading at some point is that depending on whether conhost's input thread is started or not we may end up with different behaviors on ctrl-c (apps that way be killed are not exactly the same). But that's pretty theoritical at this point. but we can sure wait for reports on this area (if any \<g\>) to more forward. preparing a simplified version of this MR -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3312#note_44381
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Jacek Caban (＠jacek)