From ea160a349d8ca045029403160707b6518492053c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 10 May 2018 18:25:01 -0400 Subject: [PATCH 1/2] Try harder to deliver SIGINT It seems that the "signal" associated with a Ctrl+C (i.e. a CTRL_C_EVENT) is sometimes not passed through to the registered handler. This symptom occurs e.g. when spawning a non-MSYS2 process in Bash through env.exe, such as is commonly the case when starting ruby scripts via the shebang line `#!/usr/bin/env ruby` (which usually calls /mingw64/bin/ruby.exe). The unexpected behavior is that a CtrlRoutine thread can be executed successfully but does not result in the process' handler to be called, let alone in the process being terminated. Even more curious is that a CTRL_BREAK_EVENT is delivered without problems. A rather intense few days of quality time with GDB and DuckDuckGo turned into the following analysis of the issue: It turns out that after calling EnterCriticalSection(), CtrlRoutine() asks whether it has been passed 0 (CTRL_C_EVENT) as a parameter and in that case, jumps to conditional code (hex addresses removed from GDB's disassembly to save on space): : callq *0x17c9af(%rip) : andl $0x0,0x24(%rsp) : test %ebx,%ebx : je That conditional code reads thusly: : mov %gs:0x60,%rax : mov 0x20(%rax),%rcx : testb $0x1,0x18(%rcx) : jne : jmp This code looks at %gs:0x60, which according to https://en.wikipedia.org/wiki/Win32_Thread_Information_Block is the PEB (Process Environment Block). Then it accesses the field of the PEB at offset 0x20, which is the ProcessParameters field according to https://msdn.microsoft.com/en-us/library/windows/desktop/aa813706.aspx These process parameters are described here: http://undocumented.ntinternals.net/UserMode/Structures/RTL_USER_PROCESS_PARAMETERS.html Sadly, it is unclear what the field at offset 0x18 (named `ConsoleFlags`) does, but from the disassembly it is clear that if it has its least significant bit set, CtrlRoutine() simply cleans up and exits. The handler(s) only get a chance to run when that bit is 0. (Note that ReactOS expects *all* of ConsoleFlags to be different from 1: https://github.com/reactos/reactos/blob/ae9702fce/dll/win32/kernel32/client/console/console.c#L169) Note: When a 64-bit process asks for the PEB of a 32-bit process, it does receive a copy of *a 64-bit version* of it. This 64-bit version points to a copy of the USER_PROCESS_INFORMATION struct that is compatible with 64-bit, but writing to it will not have any effect! We instead need to access the 32-bit PEB, which has a pointer to the 32-bit version of the process information, where we modify the ConsoleFlags. We are using a special trick to obtain the 32-bit PEB inspired by https://stackoverflow.com/a/23842609: asking NtQueryInformation() for the ProcessWow64Information will return the 64-bit address of the 32-bit PEB, if there is any (indicating that the process in question is a WoW processes i.e. 32-bit processes running on 64-bit Windows). This closes https://github.com/git-for-windows/git/issues/1648 and https://github.com/git-for-windows/git/issues/1470 Signed-off-by: Johannes Schindelin --- winsup/cygwin/include/cygwin/exit_process.h | 127 +++++++++++++++++++- 1 file changed, 125 insertions(+), 2 deletions(-) diff --git a/winsup/cygwin/include/cygwin/exit_process.h b/winsup/cygwin/include/cygwin/exit_process.h index e0981bbe4a..e6e060e93e 100644 --- a/winsup/cygwin/include/cygwin/exit_process.h +++ b/winsup/cygwin/include/cygwin/exit_process.h @@ -232,6 +232,123 @@ inject_remote_thread_into_process(HANDLE process, LPTHREAD_START_ROUTINE address return res; } +#define PROCESS_BASIC_INFORMATION_SIZE_32 0x18 +#define PEB_OFFSET_32 0x4 +#define PARAMS_OFFSET_32 0x10 +#define CONSOLE_FLAGS_OFFSET_32 0x14 + +#define PROCESS_BASIC_INFORMATION_SIZE_64 0x30 +#define PEB_OFFSET_64 0x8 +#define PARAMS_OFFSET_64 0x20 +#define CONSOLE_FLAGS_OFFSET_64 0x18 + +/** + * Adjust the ConsoleFlags to allow the process to accept Ctrl+C. + * + * The handle must be opened with PROCESS_QUERY_INFORMATION | PROCESS_VM_READ | PROCESS_WRITE. + * + * Returns 1 if the ConsoleFlags were cleared, 0 if they had already been cleared, and -1 + * on unspecified error. + */ +static int +adjust_console_flag(HANDLE process) +{ + union + { + char chars[PROCESS_BASIC_INFORMATION_SIZE_64]; + ULONG32 ulong32; + ULONG64 ulong64; + } u; + int res = 0; + char flags; + +#ifdef __LP64__ + /* Are we looking at a 32-bit process from a 64-bit one? */ + if (!NtQueryInformationProcess (process, ProcessWow64Information, &u.chars, 8, NULL) && u.ulong64) + { + SIZE_T size; + if (!ReadProcessMemory (process, (char *)u.ulong64 + PARAMS_OFFSET_32, u.chars, 4, &size) || size != 4) + return -1; + if (!ReadProcessMemory (process, (char *)(ULONG64)u.ulong32 + CONSOLE_FLAGS_OFFSET_32, &flags, 1, &size) || size != 1) + return -1; + if (flags == 1) + { + res = 1; + flags = 0; + if (!WriteProcessMemory (process, (char *)(ULONG64)u.ulong32 + CONSOLE_FLAGS_OFFSET_32, &flags, 1, &size) || size != 1) + return -1; + } + } + + if (!NtQueryInformationProcess (process, ProcessBasicInformation, u.chars, PROCESS_BASIC_INFORMATION_SIZE_64, NULL)) + { + SIZE_T size; + if (!ReadProcessMemory (process, (char *)*(ULONG64 *)(u.chars + PEB_OFFSET_64) + PARAMS_OFFSET_64, u.chars, 8, &size) || size != 8) + return -1; + if (!ReadProcessMemory (process, (char *)u.ulong64 + CONSOLE_FLAGS_OFFSET_64, &flags, 1, &size) || size != 1) + return -1; + if (flags == 1) + { + res = 1; + flags = 0; + if (!WriteProcessMemory (process, (char *)u.ulong64 + CONSOLE_FLAGS_OFFSET_64, &flags, 1, &size) || size != 1) + return -1; + } + } +#else + if (!NtQueryInformationProcess(process, ProcessBasicInformation, u.chars, PROCESS_BASIC_INFORMATION_SIZE_32, NULL) && (char *)*(ULONG32 *)(u.chars + PEB_OFFSET_32)) + { + SIZE_T size; + if (!ReadProcessMemory (process, (char *)*(ULONG32 *)(u.chars + PEB_OFFSET_32) + PARAMS_OFFSET_32, u.chars, 8, &size) || size != 8) + return -1; + if (!ReadProcessMemory (process, (char *)u.ulong32 + CONSOLE_FLAGS_OFFSET_32, &flags, 1, &size) || size != 1) + return -1; + if (flags == 1) + { + res = 1; + flags = 0; + if (!WriteProcessMemory (process, (char *)u.ulong32 + CONSOLE_FLAGS_OFFSET_32, &flags, 1, &size) || size != 1) + return -1; + } + } + + /* So maybe this is a 32-bit process looking at a 64-bit one? */ + { + HMODULE ntdll = GetModuleHandleA("ntdll.dll"); + static NTSTATUS(NTAPI *NtWow64ReadVirtualMemory64)(HANDLE process, ULONG64 address, PVOID buffer, ULONG64 size, PULONG64 count); + static NTSTATUS(NTAPI *NtWow64WriteVirtualMemory64)(HANDLE process, ULONG64 address, PVOID buffer, ULONG64 size, PULONG64 count); + static NTSTATUS(NTAPI *NtWow64QueryInformationProcess64)(HANDLE process, PROCESSINFOCLASS info_class, PVOID buffer, ULONG size, PULONG count); + static int initialized; + ULONG64 size; + + if (!initialized) + { + initialized = 1; + NtWow64ReadVirtualMemory64 = (typeof (NtWow64ReadVirtualMemory64))GetProcAddress (ntdll, "NtWow64ReadVirtualMemory64"); + NtWow64WriteVirtualMemory64 = (typeof (NtWow64WriteVirtualMemory64))GetProcAddress (ntdll, "NtWow64WriteVirtualMemory64"); + NtWow64QueryInformationProcess64 = (typeof (NtWow64QueryInformationProcess64))GetProcAddress (ntdll, "NtWow64QueryInformationProcess64"); + } + + if (NtWow64ReadVirtualMemory64 && NtWow64WriteVirtualMemory64 && NtWow64QueryInformationProcess64) + if (!NtWow64QueryInformationProcess64 (process, ProcessBasicInformation, u.chars, PROCESS_BASIC_INFORMATION_SIZE_64, NULL)) + { + if (NtWow64ReadVirtualMemory64 (process, *(ULONG64 *)(u.chars + PEB_OFFSET_64) + PARAMS_OFFSET_64, u.chars, 8, &size) || size != 8) + return -1; + if (NtWow64ReadVirtualMemory64(process, u.ulong64 + CONSOLE_FLAGS_OFFSET_64, &flags, 1, &size) || size != 1) + return -1; + } + if (flags == 1) + { + res = 1; + flags = 0; + if (NtWow64WriteVirtualMemory64(process, u.ulong64 + CONSOLE_FLAGS_OFFSET_64, &flags, 1, &size) || size != 1) + return -1; + } + } +#endif + return res; +} + /** * Terminates the process corresponding to the process ID * @@ -253,8 +370,14 @@ exit_one_process(HANDLE process, int exit_code) if (address && !inject_remote_thread_into_process(process, address, signo == SIGINT ? - CTRL_C_EVENT : CTRL_BREAK_EVENT)) - return 0; + CTRL_C_EVENT : + CTRL_BREAK_EVENT)) { + DWORD code; + if (signo == SIGINT && !GetExitCodeProcess (process, &code) || + code == STILL_ACTIVE && adjust_console_flag(process) > 0 && + !inject_remote_thread_into_process(process, address, CTRL_C_EVENT)) + return 0; + } /* fall-through */ case SIGTERM: address = get_exit_process_address_for_process(process); From e91f623439e9d8f9987b15cb063cc1450b7aed09 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 23 May 2018 10:23:10 +0200 Subject: [PATCH 2/2] TODO: use IsProcessCritical() when available Also TODO: test on 32-bit Also TODO: identify more tickets on GitHub that would probably be fixed by this topic branch Signed-off-by: Johannes Schindelin --- winsup/cygwin/include/cygwin/exit_process.h | 1 + 1 file changed, 1 insertion(+) diff --git a/winsup/cygwin/include/cygwin/exit_process.h b/winsup/cygwin/include/cygwin/exit_process.h index e6e060e93e..47d3e29601 100644 --- a/winsup/cygwin/include/cygwin/exit_process.h +++ b/winsup/cygwin/include/cygwin/exit_process.h @@ -362,6 +362,7 @@ exit_one_process(HANDLE process, int exit_code) LPTHREAD_START_ROUTINE address = NULL; int signo = exit_code & 0x7f; +TODO: use IsProcessCritical() if available (Windows 8.1 and later) to skip this switch (signo) { case SIGINT: