Skip to content

Commit

Permalink
Merge branch 'ctrl-c-with-CtrlRoutine'
Browse files Browse the repository at this point in the history
This thing again...

Background: when you hit Ctrl+C on Linux or macOS, a signal (SIGINT) is
sent to the foreground process and its child processes. This signal can
be intercepted by installing a signal handler for this specific signal.
On Windows, there is no precise equivalent for this system.

Instead, the Ctrl+C is translated by the current ConHost (i.e. the
container running the Console processes) to a ConsoleCtrl event that is
sent to all processes attached to that Console. If any of these
processes installed a handler via SetConsoleCtrlHandler(), they can
intercept that event (and avoid exiting or doing some cleanup work).

On Linux and macOS (and every Unix flavor, really), processes can also
be killed via the `kill` executable, which really just sends a signal to
the process, typically SIGTERM. Processes can intercept that signal,
too. To force processes to terminate, without giving them any chance to
prevent that, SIGKILL can be sent. There is no equivalent for SIGTERM on
Windows. To emulate SIGKILL on Windows, TerminateProcess() can be used,
but it only kills one process (unlike SIGKILL, which is sent also to the
child processes).

In Git for Windows, we struggled with emulating SIGINT, SIGTERM and
SIGKILL handling essentially since the beginning of the efforts to port
Git to Windows.

At least the SIGINT part of the problem becomes a lot worse when using a
terminal window other than cmd.exe's: as long as using cmd.exe (AKA
"ConHost"), Ctrl+C is handled entirely outside of our code. But with the
big jump from v1.x to v2.x, Git for Windows not only switched from MSys
to MSYS2, but also started using MinTTY as the default terminal window,
which uses the MSYS2 runtime-provided pseudo terminals (inherited from
Cygwin thanks to the MSYS2 runtime being a "friendly fork" of Cygwin).
When Ctrl+C is pressed in MinTTY, all of the signaling has to be done by
our code.

The original code to handle Ctrl+C comes straight from Cygwin. It simply
ignores the entire conundrum for non-Cygwin processes and simply calls
TerminateProcess() on them, leaving spawned child processes running.

The first attempt at fixing "the Ctrl+C problem" (with the symptom that
interrupting `git clone ...` would not stop the actual download of the
Git objects that was still running in a child process) was
c4ba4e3357f. It
simply enumerated all the processes' process IDs and parent process IDs
and extracted the tree of (possibly transitive) child processes of the
process to kill, then called TerminateProcess() on them.

This solved the problem with interrupting `git clone`, but it did not
address the problem that Git typically wants to "clean up" when being
interrupted. In particular, Git installs atexit() and signal handlers to
remove .lock files. The most common symptom was that a stale
.git/index.lock file was still present after interrupting a Git process.

Based on the idea presented in Dr Dobb's Journal in the article "A Safer
Alternative to TerminateProcess()" by Andrew Tucker (July 1, 1999)
http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547
we changed our handling to inject a remote thread calling ExitProcess()
first, and fall back to TerminateProcess() the process tree instead:
e9cb332976c

This change was a little misguided in hindsight, as it only called
TerminateProcess() on the process tree, but expected the atexit()
handler of Git to take care of the child processes when killing the
process via the remote ExitProcess() method.

Therefore, we changed the strategy once again, to inject ExitProcess()
threads into the child processes of the process to kill, too:
53e5c0313e1

(That commit also tries to handle Cygwin process among the child
processes by sending Cygwin signals, but unfortunately that part of the
commit was buggy.)

This worked well for Git processes. However, Git Bash is used in all
kinds of circumstances, including launching Maven, or node.js scripts
that want to intercept SIGINT. Naturally, these callees have no idea
that Git for Windows injects an ExitProcess() with exit code 130
(corresponding to 0x100 + SIGINT). Therefore, they never "got" the
signal.

So what is it that happens when ConHost generates a ConsoleCtrl event?
This question was asked and answered in the excellent blog post at:
http://stanislavs.org/stopping-command-line-applications-programatically-with-ctrl-c-events-from-net/#comment-2880

Essentially, the same happens as what we did with ExitProcess(): a
remote thread gets injected, with the event type as parameter. Of course
it is not ExitProcess() that is called, but CtrlRoutine(). This function
lives in kernel32.dll, too, but it is not exported, i.e.
GetProcAddress() won't find it. The trick proposed in the blog post (to
send a test ConsoleCtrl event to the process itself, using a special
handler that then inspects the stack trace to figure out the address of
the caller) does not work for us, however: it would send a
CTRL_BREAK_EVENT to *all* processes attached to the same Console,
essentially killing MinTTY.

But could we make this still work somehow? Yes, we could. We came up
with yet another trick up our sleeves: instead of determining the
address of kernel32!CtrlRoutine in our own process, we spawn a new one,
with a new Console, to avoid killing MinTTY. To do that, we need a
helper .exe, of course, which we put into /usr/libexec/. If this helper
is not found, we fall back to the previous methods of injecting
ExitProcess() or calling TerminateProcess().

This method (to spawn a helper .exe) has a further incidental benefit:
by compiling 32-bit *and* 64-bit helpers and providing them as
getprocaddr32.exe and getprocaddr64.exe, we can now also handle 32-bit
processes in a 64-bit Git for Windows. Sadly not vice versa: calling
CreateRemoteThread() on a 64-bit process from a 32-bit process seems to
fail all the time (and require a lot of assembly hackery to fix that I
am not really willing to include in Git for Windows' MSYS2 runtime).

The current method was implemented in this commit:
ca6188a7520

This is the hopeful final fix for
git-for-windows/git#1491,
git-for-windows/git#1470,
git-for-windows/git#1248,
git-for-windows/git#1239,
git-for-windows/git#227,
git-for-windows/git#1553,
nodejs/node#16103, and plenty other tickets
that petered out mostly due to a willingness of community members to
leave all the hard work to a single, already overworked person.

This fix also partially helps
git-for-windows/git#1629 (only partially
because the user wanted to quit the pager using Ctrl+C, which is not the
intended consequence of a Ctrl+C: it should stop the Git process, but
not the pager).

Signed-off-by: Johannes Schindelin <[email protected]>
  • Loading branch information
dscho committed Apr 23, 2018
2 parents 50159fe + c87a176 commit 78e2dee
Show file tree
Hide file tree
Showing 7 changed files with 553 additions and 91 deletions.
4 changes: 2 additions & 2 deletions winsup/cygwin/exceptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1561,8 +1561,8 @@ sigpacket::process ()
goto done;
default:
sigproc_printf ("terminating captive process");
exit_process (ch_spawn, 128 + (sigExeced = si.si_signo), 0);
break;
rc = exit_process_tree (ch_spawn, 128 + (sigExeced = si.si_signo));
goto done;
}
}
/* Dispatch to the appropriate function. */
Expand Down
Loading

0 comments on commit 78e2dee

Please sign in to comment.