From ea160a349d8ca045029403160707b6518492053c Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
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):
<KERNELBASE!CtrlRoutine+91>: callq *0x17c9af(%rip)
<KERNELBASE!CtrlRoutine+97>: andl $0x0,0x24(%rsp)
<KERNELBASE!CtrlRoutine+102>: test %ebx,%ebx
<KERNELBASE!CtrlRoutine+104>: je <KERNELBASE!CtrlRoutine+163>
That conditional code reads thusly:
<KERNELBASE!CtrlRoutine+163>: mov %gs:0x60,%rax
<KERNELBASE!CtrlRoutine+172>: mov 0x20(%rax),%rcx
<KERNELBASE!CtrlRoutine+176>: testb $0x1,0x18(%rcx)
<KERNELBASE!CtrlRoutine+180>: jne <KERNELBASE!CtrlRoutine+216>
<KERNELBASE!CtrlRoutine+182>: jmp <KERNELBASE!CtrlRoutine+106>
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 <johannes.schindelin@gmx.de>
---
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 <johannes.schindelin@gmx.de>
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 <johannes.schindelin@gmx.de>
---
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: