After "mingw: kill child processes in a gentler way", the ownership of
the HANDLE passed to `exit_process()` and `terminate_process_tree()` is
inconsistent. `terminate_process_tree()` always closes the handle;
`exit_process()` closes it on success and on the terminate-tree
fallback, but leaks it on the early return where GetExitCodeProcess()
fails or reports the process is no longer STILL_ACTIVE.
`mingw_kill()` compensated by closing the handle on its own error path,
which is a double-close on every error path that does not hit that one
leaky branch -- the callee has already closed the handle by then.
Coverity flagged the resulting use-after-free as CID 1437238.
Pin down the invariant that `exit_process()` and
`terminate_process_tree()` own the handle from the call onward and close
it on every return path; with that, the bogus close in `mingw_kill()`
goes away.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The TerminateProcess() function does not actually leave the child
processes any chance to perform any cleanup operations. This is bad
insofar as Git itself expects its signal handlers to run.
A symptom is e.g. a left-behind .lock file that would not be left behind
if the same operation was run, say, on Linux.
To remedy this situation, we use an obscure trick: we inject a thread
into the process that needs to be killed and to let that thread run the
ExitProcess() function with the desired exit status. Thanks J Wyman for
describing this trick.
The advantage is that the ExitProcess() function lets the atexit
handlers run. While this is still different from what Git expects (i.e.
running a signal handler), in practice Git sets up signal handlers and
atexit handlers that call the same code to clean up after itself.
In case that the gentle method to terminate the process failed, we still
fall back to calling TerminateProcess(), but in that case we now also
make sure that processes spawned by the spawned process are terminated;
TerminateProcess() does not give the spawned process a chance to do so
itself.
Please note that this change only affects how Git for Windows tries to
terminate processes spawned by Git's own executables. Third-party
software that *calls* Git and wants to terminate it *still* need to make
sure to imitate this gentle method, otherwise this patch will not have
any effect.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johannes Schindelin