WIP Try harder to handle Ctrl+C #18
Conversation
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 git-for-windows/git#1648 and git-for-windows/git#1470 Signed-off-by: Johannes Schindelin <[email protected]>
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 <[email protected]>
|
@orgads this is the PR. Please have a look... |
|
@orgads I really could use your help... |
|
Sure! I didn't forget you, I just had a heavy workload in the past few days. I'll try to test it by Sunday. |
Thank you! |
|
I had to comment the This solves the (before this change, |
|
Any progress with this? |
Unfortunately not. It has been on the back burner and will remain there for the time being. :-( |
|
Do you plan to complete this commit? This bug is the most annoying one for me. It looks like it is 80-90% done, isn't it? |
Hmm, I seem to not find the time to finish this any time soon.
The biggest problem here is that it might make Git Bash look like malware to some anti-malware software, as it digs pretty deep into internals of other processes. So I started wondering whether this is really the right approach, and whether we should not rather have a simpler (and more documented, although not quite as satisfying) strategy of trying to kill the process with a remote thread, then see whether the process is still around, falling back to terminating it. This strategy would not be quite as satisfying because it would fail to emulate the Ctrl+C behavior in CMD. Any thoughts? |
It has been reported in git-for-windows/git#1648 that our way to simulate
SIGINTsometimes fails to do anything.This PR fixes that. See the commit message for an extensive description of the culprit and its resolution.
TODOs before merging:
Explain in the commit message that the most likely reason that Ctrl+C is ignored in those situations is described in https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425.aspx:
Test with a 32-bit runtime, in particular whether it manages to interrupt a 64-bit process.
Load the
IsProcessCritical()symbol, and where available (i.e. Windows 8.1 and later), use it to avoid interrupting processes that should never be interrupted.Identify more tickets that would benefit from this fix, and mention them in the commit message, too.