From 82014d110c375177ecfb7bddff66aeb6c42fcb7c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 17 May 2017 14:28:59 +0200 Subject: [PATCH 1/3] fixup! Export the `kill_process_tree()` function This drops the patch. After making up my mind about it, it would appear to be a bad idea to export this via the .dll. Better implement the functions in a header file that needs to be #include'd by the callers, and marks the functions as file-local. Signed-off-by: Johannes Schindelin --- winsup/cygwin/common.din | 1 - winsup/cygwin/include/cygwin/signal.h | 3 +-- winsup/cygwin/include/cygwin/version.h | 3 +-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din index 11c91c627a..809b88ba61 100644 --- a/winsup/cygwin/common.din +++ b/winsup/cygwin/common.din @@ -33,7 +33,6 @@ sys_errlist = _sys_errlist DATA sys_nerr = _sys_nerr DATA sys_sigabbrev DATA sys_siglist DATA -kill_process_tree DATA # Exported functions _Exit SIGFE diff --git a/winsup/cygwin/include/cygwin/signal.h b/winsup/cygwin/include/cygwin/signal.h index 6c636dde85..ae86de0127 100644 --- a/winsup/cygwin/include/cygwin/signal.h +++ b/winsup/cygwin/include/cygwin/signal.h @@ -410,12 +410,11 @@ int siginterrupt (int, int); #ifdef __INSIDE_CYGWIN__ extern const char *sys_sigabbrev[]; extern const char *sys_siglist[]; -extern void kill_process_tree(pid_t pid, int sig); #else extern const char __declspec(dllimport) *sys_sigabbrev[]; extern const char __declspec(dllimport) *sys_siglist[]; -extern void __declspec(dllimport) kill_process_tree(pid_t pid, int sig); #endif +void kill_process_tree(pid_t pid, int sig); #ifdef __cplusplus } diff --git a/winsup/cygwin/include/cygwin/version.h b/winsup/cygwin/include/cygwin/version.h index a73e820d8b..2915bf811f 100644 --- a/winsup/cygwin/include/cygwin/version.h +++ b/winsup/cygwin/include/cygwin/version.h @@ -474,13 +474,12 @@ details. */ 307: Export timingsafe_bcmp, timingsafe_memcmp. 308: Export dladdr. 309: Export getloadavg. - 310: Export kill_process_tree. Note that we forgot to bump the api for ualarm, strtoll, strtoull, sigaltstack, sethostname. */ #define CYGWIN_VERSION_API_MAJOR 0 -#define CYGWIN_VERSION_API_MINOR 310 +#define CYGWIN_VERSION_API_MINOR 309 /* There is also a compatibity version number associated with the shared memory regions. It is incremented when incompatible changes are made to the shared From e9cb332976cf6ba44d9f5fc0ed4f725ce43fe646 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 20 Mar 2015 09:56:28 +0000 Subject: [PATCH 2/3] squash! Kill also children of the process to be killed via Ctrl+C Try to kill Win32 processes gently upon Ctrl+C When a process is terminated via TerminateProcess(), it has no chance to do anything in the way of cleaning up. This is particularly noticeable when a lengthy Git for Windows process tries to update Git's index file and leaves behind an index.lock file. Git's idea is to remove the stale index.lock file in that case, using the signal and atexit handlers available in Linux. But those signal handlers never run. Note: this is not an issue for MSYS2 processes because MSYS2 emulates Unix' signal system accurately, both for the process sending the kill signal and the process receiving it. Win32 processes do not have such a signal handler, though, instead MSYS2 shuts them down via `TerminateProcess()`. Let's use a gentler method, described in the Dr Dobb's article "A Safer Alternative to TerminateProcess()" by Andrew Tucker (July 1, 1999), http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547 Essentially, we inject a new thread into the running process that does nothing else than running the ExitProcess() function. If that fails, or when the process still lives on after 10 seconds, we fall back to calling TerminateProcess(), but in that case we take pains to kill also processes directly or indirectly spawned by that process; TerminateProcess() does not give any process a chance to terminate its child processes. Originally, we wanted to call the function `GenerateConsoleCtrlEvent()` for SIGTERM with console processes, but 1) that may not work as it requires a process *group* ID (i.e. the process ID of the initial process group member, which we may not have), and 2) it does not kill the process *tree* on Windows 7 and earlier. Note: we do not use the NtQueryInformationProcess() function because 1) it is marked internal and subject to change at any time of Microsoft's choosing, and 2) it does not even officially report the child/parent relationship (the pid of the parent process is stored in the `Reserved3` slot of the `PROCESS_BASIC_INFORMATION` structure). Instead, we resort to the Toolhelp32 API -- which happily also works on 64-bit Windows -- to enumerate the process tree and reconstruct the process tree rooted in the process we intend to kill. While at it, we also fix the exit status: the convention is to add 128 to the signal number, to give exit code handlers a chance to detect an "exit by signal" condition. This fixes the MSYS2 side of the bug where interrupting `git clone https://...` would send the spawned-off `git remote-https` process into the background instead of interrupting it, i.e. the clone would continue and its progress would be reported mercilessly to the console window without the user being able to do anything about it (short of firing up the task manager and killing the appropriate task manually). Note that this special-handling is only necessary when *MSYS2* handles the Ctrl+C event, e.g. when interrupting a process started from within MinTTY or any other non-cmd-based terminal emulator. If the process was started from within `cmd.exe`'s terminal window, child processes are already killed appropriately upon Ctrl+C. Signed-off-by: Johannes Schindelin --- winsup/cygwin/exceptions.cc | 6 +- winsup/cygwin/include/cygwin/exit_process.h | 170 ++++++++++++++++++++ winsup/cygwin/signal.cc | 57 ------- 3 files changed, 172 insertions(+), 61 deletions(-) create mode 100644 winsup/cygwin/include/cygwin/exit_process.h diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 6707eb9f5f..73a16ac058 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -27,6 +27,7 @@ details. */ #include "child_info.h" #include "ntdll.h" #include "exception.h" +#include "cygwin/exit_process.h" /* Definitions for code simplification */ #ifdef __x86_64__ @@ -1547,10 +1548,7 @@ sigpacket::process () if (have_execed) { sigproc_printf ("terminating captive process"); - if ((sigExeced = si.si_signo) == SIGINT) - kill_process_tree (GetProcessId (ch_spawn), sigExeced = si.si_signo); - else - TerminateProcess (ch_spawn, sigExeced = si.si_signo); + exit_process (ch_spawn, 128 + (sigExeced = si.si_signo)); } /* Dispatch to the appropriate function. */ sigproc_printf ("signal %d, signal handler %p", si.si_signo, handler); diff --git a/winsup/cygwin/include/cygwin/exit_process.h b/winsup/cygwin/include/cygwin/exit_process.h new file mode 100644 index 0000000000..ca75535b2b --- /dev/null +++ b/winsup/cygwin/include/cygwin/exit_process.h @@ -0,0 +1,170 @@ +#ifndef EXIT_PROCESS_H +#define EXIT_PROCESS_H + +/* + * This file contains functions to terminate a Win32 process, as gently as + * possible. + * + * At first, we will attempt to inject a thread that calls ExitProcess(). If + * that fails, we will fall back to terminating the entire process tree. + * + * As we do not want to export this function in the MSYS2 runtime, these + * functions are marked as file-local. + */ + +#include + +/** + * Terminates the process corresponding to the process ID and all of its + * directly and indirectly spawned subprocesses. + * + * This way of terminating the processes is not gentle: the processes get + * no chance of cleaning up after themselves (closing file handles, removing + * .lock files, terminating spawned processes (if any), etc). + */ +static int +terminate_process_tree(HANDLE main_process, int exit_code) +{ + HANDLE snapshot = CreateToolhelp32Snapshot (TH32CS_SNAPPROCESS, 0); + PROCESSENTRY32 entry; + DWORD pids[16384]; + int max_len = sizeof (pids) / sizeof (*pids), i, len, ret = 0; + pid_t pid = GetProcessId (main_process); + + pids[0] = (DWORD) pid; + len = 1; + + /* + * Even if Process32First()/Process32Next() seem to traverse the + * processes in topological order (i.e. parent processes before + * child processes), there is nothing in the Win32 API documentation + * suggesting that this is guaranteed. + * + * Therefore, run through them at least twice and stop when no more + * process IDs were added to the list. + */ + for (;;) + { + int orig_len = len; + + memset (&entry, 0, sizeof (entry)); + entry.dwSize = sizeof (entry); + + if (!Process32First (snapshot, &entry)) + break; + + do + { + for (i = len - 1; i >= 0; i--) + { + if (pids[i] == entry.th32ProcessID) + break; + if (pids[i] == entry.th32ParentProcessID) + pids[len++] = entry.th32ProcessID; + } + } + while (len < max_len && Process32Next (snapshot, &entry)); + + if (orig_len == len || len >= max_len) + break; + } + + for (i = len - 1; i >= 0; i--) + { + HANDLE process = i == 0 ? main_process : + OpenProcess (PROCESS_TERMINATE, FALSE, pids[i]); + + if (process) + { + if (!TerminateProcess (process, exit_code)) + ret = -1; + CloseHandle (process); + } + } + + return ret; +} + +/** + * Determine whether a process runs in the same architecture as the current + * one. That test is required before we assume that GetProcAddress() returns + * a valid address *for the target process*. + */ +static inline bool +process_architecture_matches_current(HANDLE process) +{ + static BOOL current_is_wow = -1; + BOOL is_wow; + + if (current_is_wow == -1 && + !IsWow64Process (GetCurrentProcess (), ¤t_is_wow)) + current_is_wow = -2; + if (current_is_wow == -2) + return false; /* could not determine current process' WoW-ness */ + if (!IsWow64Process (process, &is_wow)) + return false; /* cannot determine */ + return is_wow == current_is_wow; +} + +/** + * Inject a thread into the given process that runs ExitProcess(). + * + * Note: as kernel32.dll is loaded before any process, the other process and + * this process will have ExitProcess() at the same address. + * + * This function expects the process handle to have the access rights for + * CreateRemoteThread(): PROCESS_CREATE_THREAD, PROCESS_QUERY_INFORMATION, + * PROCESS_VM_OPERATION, PROCESS_VM_WRITE, and PROCESS_VM_READ. + * + * The idea comes from the Dr Dobb's article "A Safer Alternative to + * TerminateProcess()" by Andrew Tucker (July 1, 1999), + * http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547 + * + * If this method fails, we fall back to running terminate_process_tree(). + */ +static int +exit_process(HANDLE process, int exit_code) +{ + DWORD code; + + if (GetExitCodeProcess (process, &code) && code == STILL_ACTIVE) + { + /* + * We cannot determine the address of ExitProcess() for a process + * that does not match the current architecture (e.g. for a 32-bit + * process when we're running in 64-bit mode). + */ + if (process_architecture_matches_current (process)) + { + static LPTHREAD_START_ROUTINE exit_process_address; + if (!exit_process_address) + { + HINSTANCE kernel32 = GetModuleHandle ("kernel32"); + exit_process_address = (LPTHREAD_START_ROUTINE) + GetProcAddress (kernel32, "ExitProcess"); + } + DWORD thread_id; + HANDLE thread = !exit_process_address ? NULL : + CreateRemoteThread (process, NULL, 0, exit_process_address, + (PVOID)exit_code, 0, &thread_id); + + if (thread) + { + CloseHandle (thread); + /* + * Wait 10 seconds (arbitrary constant) for the process to + * finish; After that grace period, fall back to terminating + * non-gently. + */ + if (WaitForSingleObject (process, 10000) == WAIT_OBJECT_0) + return 0; + } + } + + return terminate_process_tree (process, exit_code); + } + + return -1; +} + +#endif diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc index 45cf5d4c46..f371a231bb 100644 --- a/winsup/cygwin/signal.cc +++ b/winsup/cygwin/signal.cc @@ -10,7 +10,6 @@ Cygwin license. Please consult the file "CYGWIN_LICENSE" for details. */ #include "winsup.h" -#include #include #include #include "pinfo.h" @@ -359,62 +358,6 @@ killpg (pid_t pgrp, int sig) return kill (-pgrp, sig); } -/** - * Terminates the process corresponding to the process ID and all of its - * directly and indirectly spawned subprocesses. - */ -extern "C" void -kill_process_tree(pid_t pid, int sig) -{ - HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); - PROCESSENTRY32 entry; - DWORD pids[16384]; - int max_len = sizeof(pids) / sizeof(*pids), i, len; - - pids[0] = (DWORD) pid; - len = 1; - - /* - * Even if Process32First()/Process32Next() seem to traverse the - * processes in topological order (i.e. parent processes before - * child processes), there is nothing in the Win32 API documentation - * suggesting that this is guaranteed. - * - * Therefore, run through them at least twice and stop when no more - * process IDs were added to the list. - */ - for (;;) { - int orig_len = len; - - memset(&entry, 0, sizeof(entry)); - entry.dwSize = sizeof(entry); - - if (!Process32First(snapshot, &entry)) - break; - - do { - for (i = len - 1; i >= 0; i--) { - if (pids[i] == entry.th32ProcessID) - break; - if (pids[i] == entry.th32ParentProcessID) - pids[len++] = entry.th32ProcessID; - } - } while (len < max_len && Process32Next(snapshot, &entry)); - - if (orig_len == len || len >= max_len) - break; - } - - for (i = len - 1; i >= 0; i--) { - HANDLE process = OpenProcess(PROCESS_TERMINATE, FALSE, pids[i]); - - if (process) { - TerminateProcess(process, sig << 8); - CloseHandle(process); - } - } -} - extern "C" void abort (void) { From f4a52b9fa050a659fc289d94071429081ce299f5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 20 Mar 2015 10:01:50 +0000 Subject: [PATCH 3/3] squash! kill: Handle Win32 console processes' children, too kill: kill Win32 processes more gently This change is the equivalent to the change to the Ctrl+C handling we just made. Signed-off-by: Johannes Schindelin --- winsup/utils/kill.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/winsup/utils/kill.cc b/winsup/utils/kill.cc index 6698f912be..734e656aa5 100644 --- a/winsup/utils/kill.cc +++ b/winsup/utils/kill.cc @@ -17,6 +17,7 @@ details. */ #include #include #include +#include static char *prog_name; @@ -173,7 +174,21 @@ forcekill (int pid, int sig, int wait) if (!wait || WaitForSingleObject (h, 200) != WAIT_OBJECT_0) { if (sig == SIGINT || sig == SIGTERM) - kill_process_tree (dwpid, sig); + { + HANDLE cur = GetCurrentProcess (), h2; + /* duplicate handle with access rights required for exit_process() */ + if (DuplicateHandle (cur, h, cur, &h2, PROCESS_CREATE_THREAD | + PROCESS_QUERY_INFORMATION | + PROCESS_VM_OPERATION | + PROCESS_VM_WRITE | PROCESS_VM_READ | + PROCESS_TERMINATE, FALSE, 0)) + { + exit_process (h2, 128 + sig); + CloseHandle (h2); + } + else + terminate_process_tree(h, 128 + sig); + } else if (sig && !TerminateProcess (h, sig << 8) && WaitForSingleObject (h, 200) != WAIT_OBJECT_0) fprintf (stderr, "%s: couldn't kill pid %u, %u\n",