fix(windows): spawn git without inheriting handles (UI hang)#846
Merged
Conversation
Windows cbm_popen wrapped _popen, whose CreateProcess(bInheritHandles=TRUE) leaks every inheritable handle - including the UI listening socket and Winsock/AFD helpers - into git children. Git-for-Windows' MSYS2 runtime walks inherited handles with NtQueryObject at startup and deadlocks on socket/AFD handles, wedging the single-threaded UI server (#798). Read-mode cbm_popen now spawns via CreateProcessW with STARTUPINFOEXW and PROC_THREAD_ATTRIBUTE_HANDLE_LIST restricted to the stdout pipe write end and a NUL handle for stdin/stderr; cbm_pclose reaps the child via a handle table + WaitForSingleObject + GetExitCodeProcess. Distilled from #799 with three corrections: - no silent fallback to _popen: a failed isolated spawn logs a structured warning (compat.popen_isolated_failed stage/gle/errno) and returns NULL (all call sites handle NULL); the composed command line is heap-allocated instead of a fixed 2048-byte buffer whose overflow forced the fallback - cmd.exe resolved explicitly from %COMSPEC% (GetSystemDirectoryW\cmd.exe fallback) and passed as lpApplicationName - no search-path lookup - GetExitCodeProcess failure reports -1 instead of success; a failed NUL open fails the spawn instead of feeding INVALID_HANDLE_VALUE into STARTF_USESTDHANDLES Adds two windows-only regression tests (test_security.c) driving the cbm_popen/cbm_pclose round-trip against real git and asserting the isolated path was taken via the cbm_popen_last_was_isolated() hook. Closes #798 Co-authored-by: Flipper <jacobphilipp@ymail.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(windows): spawn git without inheriting handles (UI hang)
Closes #798.
Distilled from #799 (thanks @Flipper1994) with three review corrections.
Mechanism
On Windows,
cbm_popenwrapped the CRT's_popen, which callsCreateProcess(bInheritHandles=TRUE): every inheritable handle in the parent— the UI listening socket, Winsock/AFD helper handles, the MCP stdio pipe —
leaks into the child. When the child is git-for-Windows, its MSYS2/Cygwin
runtime walks all inherited handles at startup and calls
NtQueryObjectoneach to classify it; on an inherited socket/AFD handle that call deadlocks.
The UI server handles requests on a single thread, so one wedged
gitchild (e.g.list_projectsshelling out per project) hangs the wholeweb UI.
The fix replaces the read-mode
_popenpath with an isolated spawn:CreatePipe(inheritable SA) +SetHandleInformationde-inherits theparent read end;
STARTUPINFOEXW+PROC_THREAD_ATTRIBUTE_HANDLE_LISTrestricted toexactly two handles: the stdout write end and a
NULhandle forstdin/stderr (
STARTF_USESTDHANDLES);cmd.exe /cso quoting and2>NULredirectionsbehave exactly as under
_popen;cbm_pcloseresolves the stream via a small handle table and reaps thechild with
WaitForSingleObject+GetExitCodeProcess(
INIT_ONCE-guarded critical section).Nothing foreign crosses into git, so there is no handle to deadlock on.
POSIX
popenalready setsO_CLOEXEC; that path is untouched.Corrections applied on top of #799
_popen— the original fell back to_popenwhenever the isolated spawn failed, silently re-arming the deadlock. Now a
failure logs a structured warning
(
compat.popen_isolated_failed stage=... gle=... errno=...) and returnsNULL; all 17
cbm_popencall sites already handle NULL. The composedcommand line is heap-allocated (the original used a fixed 2048-byte buffer
whose overflow forced the fallback) and widened via UTF-8 so non-ASCII
repo paths survive.
cmd.exeresolution — the shell is resolved from%COMSPEC%(
GetEnvironmentVariableW), falling back toGetSystemDirectoryW+\cmd.exe, and passed aslpApplicationNamesoCreateProcessWneverwalks the search path (no
cmd.exeplanting from a hostile CWD).GetExitCodeProcessfailure nowreturns -1 instead of reporting success; a failed
NULopen fails thespawn instead of passing
INVALID_HANDLE_VALUEintoSTARTF_USESTDHANDLESslots.Tests
Two Windows-only regression tests in
tests/test_security.c(suite
security, windows-latest CI):popen_isolated_git_version_round_trip—cbm_popen("git --version", "r")returns output through the isolated spawn andcbm_pclosereturns 0.Asserts via the
cbm_popen_last_was_isolated()test hook that the streamcame from the isolated path — a revert to raw
_popenfails the guard.popen_isolated_propagates_exit_code— child exit code 7 surfaces throughGetExitCodeProcess.Verification
make -f Makefile.cbm cbm(-Werror) clean — POSIX path untouched;lint-ciclean; full C test suite green (Windows tests compiled out)._WIN32block, the twonew regression tests against real git-for-Windows, and the absence of the
hang in practice. The full UI-mode repro (listening socket + MSYS2 handle
walk under a single-threaded server) is not covered by these tests and is
noted as a follow-up harness.