diff --git a/src/foundation/compat_fs.c b/src/foundation/compat_fs.c index c08d7612..f1b94c0d 100644 --- a/src/foundation/compat_fs.c +++ b/src/foundation/compat_fs.c @@ -21,7 +21,11 @@ #endif #include #include /* _wmkdir */ -#include /* _wunlink */ +#include /* errno for spawn-failure logging */ +#include /* _O_RDONLY */ +#include /* _wunlink, _open_osfhandle, _close */ +#include /* intptr_t */ +#include "foundation/log.h" #include "foundation/win_utf8.h" struct cbm_dir { @@ -123,12 +127,253 @@ void cbm_closedir(cbm_dir_t *d) { } } +/* Windows _popen replacement that inherits ONLY the child's stdout pipe. + * + * The CRT's _popen uses CreateProcess(bInheritHandles=TRUE), which leaks EVERY + * inheritable handle we hold into the child — listening/client sockets, the + * Winsock/AFD helper handles created by WSAStartup, the MCP stdio pipe, etc. + * When the child is git-for-Windows (MSYS2/Cygwin runtime), its startup walks + * every inherited handle and calls NtQueryObject on each to classify it; on an + * inherited socket/AFD handle NtQueryObject deadlocks. Since our UI server runs + * requests on a single thread, that wedges the whole server (list_projects, + * which shells out to git per project, never returns → the web UI hangs). + * + * The fix: spawn via CreateProcessW with STARTUPINFOEXW + an explicit + * PROC_THREAD_ATTRIBUTE_HANDLE_LIST containing only the stdout write-end and a + * NUL handle for stdin/stderr. Nothing else crosses into git, so there is no + * foreign handle to deadlock on. POSIX popen() already sets O_CLOEXEC on its + * pipe, so the POSIX path is unchanged. + * + * There is deliberately NO fallback to _popen when the isolated spawn fails: + * falling back would silently re-arm the deadlock. cbm_popen logs a structured + * warning and returns NULL instead (every call site handles NULL). */ + +enum { CBM_POPEN_MAX = 16 }; +static struct { + FILE *fp; + HANDLE proc; +} g_popen_tab[CBM_POPEN_MAX]; +static CRITICAL_SECTION g_popen_lock; +static INIT_ONCE g_popen_once = INIT_ONCE_STATIC_INIT; + +/* Test hook (declared in compat_fs_internal.h): 1 when the most recent + * cbm_popen(..., "r") stream came from the isolated spawn. Test-only + * observable; not synchronized across threads. */ +static volatile LONG g_popen_last_isolated = 0; + +int cbm_popen_last_was_isolated(void) { + return (int)g_popen_last_isolated; +} + +static BOOL CALLBACK cbm_popen_init(PINIT_ONCE once, PVOID param, PVOID *ctx) { + (void)once; + (void)param; + (void)ctx; + InitializeCriticalSection(&g_popen_lock); + return TRUE; +} + +/* Resolve the shell explicitly — %COMSPEC%, else \cmd.exe — so it + * can be passed as lpApplicationName and CreateProcess never walks the search + * path (no cmd.exe planting from a hostile CWD). Heap string; caller frees. */ +static wchar_t *cbm_resolve_comspec(void) { + wchar_t buf[MAX_PATH]; + const wchar_t suffix[] = L"\\cmd.exe"; + DWORD n = GetEnvironmentVariableW(L"COMSPEC", buf, MAX_PATH); + if (n == 0 || n >= MAX_PATH) { + UINT sn = GetSystemDirectoryW(buf, MAX_PATH); + if (sn == 0 || (size_t)sn + wcslen(suffix) >= MAX_PATH) { + return NULL; + } + wmemcpy(buf + sn, suffix, wcslen(suffix) + 1); + } + return _wcsdup(buf); +} + +/* On failure returns NULL with *stage naming the failing step and *gle the + * GetLastError value captured at that step (0 when errno is the signal). */ +static FILE *cbm_popen_isolated(const char *cmd, const char **stage, DWORD *gle) { + *stage = ""; + *gle = 0; + InitOnceExecuteOnce(&g_popen_once, cbm_popen_init, NULL, NULL); + + SECURITY_ATTRIBUTES sa; + sa.nLength = sizeof(sa); + sa.lpSecurityDescriptor = NULL; + sa.bInheritHandle = TRUE; + + HANDLE rd = NULL, wr = NULL; + if (!CreatePipe(&rd, &wr, &sa, 0)) { + *stage = "pipe"; + *gle = GetLastError(); + return NULL; + } + /* The parent read-end must never cross into the child. */ + SetHandleInformation(rd, HANDLE_FLAG_INHERIT, 0); + + /* NUL for the child's stdin/stderr so it never touches our real stdin + * pipe. If NUL cannot be opened, fail: STARTF_USESTDHANDLES slots must + * never carry INVALID_HANDLE_VALUE. */ + HANDLE nul = CreateFileW(L"NUL", GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, OPEN_EXISTING, 0, NULL); + if (nul == INVALID_HANDLE_VALUE) { + *stage = "nul"; + *gle = GetLastError(); + CloseHandle(rd); + CloseHandle(wr); + return NULL; + } + + HANDLE inherit[2]; + inherit[0] = wr; + inherit[1] = nul; + + SIZE_T attr_sz = 0; + InitializeProcThreadAttributeList(NULL, 1, 0, &attr_sz); + LPPROC_THREAD_ATTRIBUTE_LIST attr = (LPPROC_THREAD_ATTRIBUTE_LIST)malloc(attr_sz); + BOOL attr_init = attr && InitializeProcThreadAttributeList(attr, 1, 0, &attr_sz); + BOOL prepared = + attr_init && UpdateProcThreadAttribute(attr, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherit, + sizeof(inherit), NULL, NULL); + DWORD attr_gle = prepared ? 0 : GetLastError(); + + STARTUPINFOEXW si; + ZeroMemory(&si, sizeof(si)); + si.StartupInfo.cb = sizeof(si); + si.StartupInfo.dwFlags = STARTF_USESTDHANDLES; + si.StartupInfo.hStdInput = nul; + si.StartupInfo.hStdOutput = wr; + si.StartupInfo.hStdError = nul; + si.lpAttributeList = attr; + + /* Run through cmd.exe /c so command quoting and `2>NUL` behave as under + * _popen. The command line is heap-composed (no fixed-size truncation) + * and widened via UTF-8 so non-ASCII repo paths survive intact. */ + wchar_t *app = cbm_resolve_comspec(); + wchar_t *wcmdline = NULL; + if (app) { + size_t u8len = strlen(cmd) + sizeof("cmd.exe /c "); + char *u8 = (char *)malloc(u8len); + if (u8) { + snprintf(u8, u8len, "cmd.exe /c %s", cmd); + wcmdline = cbm_utf8_to_wide(u8); + free(u8); + } + } + + PROCESS_INFORMATION pi; + ZeroMemory(&pi, sizeof(pi)); + BOOL created = FALSE; + if (!prepared) { + *stage = "attr"; + *gle = attr_gle; + } else if (!app || !wcmdline) { + *stage = "cmdline"; + *gle = ERROR_NOT_ENOUGH_MEMORY; + } else { + created = CreateProcessW(app, wcmdline, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, &si.StartupInfo, &pi); + if (!created) { + *stage = "spawn"; + *gle = GetLastError(); + } + } + + free(app); + free(wcmdline); + if (attr) { + if (attr_init) { + DeleteProcThreadAttributeList(attr); + } + free(attr); + } + CloseHandle(wr); /* the child owns the write-end now */ + CloseHandle(nul); + if (!created) { + CloseHandle(rd); + return NULL; + } + CloseHandle(pi.hThread); + + int fd = _open_osfhandle((intptr_t)rd, _O_RDONLY); + if (fd == -1) { + *stage = "osfhandle"; + CloseHandle(rd); + CloseHandle(pi.hProcess); + return NULL; + } + FILE *fp = _fdopen(fd, "r"); /* takes ownership of fd/rd */ + if (!fp) { + *stage = "fdopen"; + _close(fd); + CloseHandle(pi.hProcess); + return NULL; + } + + EnterCriticalSection(&g_popen_lock); + for (int i = 0; i < CBM_POPEN_MAX; i++) { + if (!g_popen_tab[i].fp) { + g_popen_tab[i].fp = fp; + g_popen_tab[i].proc = pi.hProcess; + LeaveCriticalSection(&g_popen_lock); + return fp; + } + } + LeaveCriticalSection(&g_popen_lock); + /* Table full (shouldn't happen): don't leak the process handle. */ + *stage = "table"; + CloseHandle(pi.hProcess); + fclose(fp); + return NULL; +} + FILE *cbm_popen(const char *cmd, const char *mode) { + /* Our git shell-outs are all read-mode; they MUST use the isolated + * spawn. On failure, log and fail the call — never fall back to + * _popen, whose full handle inheritance re-arms the UI hang (#798). */ + if (mode && mode[0] == 'r' && mode[1] == '\0') { + const char *stage = ""; + DWORD gle = 0; + FILE *fp = cbm_popen_isolated(cmd, &stage, &gle); + g_popen_last_isolated = (fp != NULL); + if (!fp) { + char glebuf[CBM_SZ_16]; + char errnobuf[CBM_SZ_16]; + snprintf(glebuf, sizeof(glebuf), "%lu", (unsigned long)gle); + snprintf(errnobuf, sizeof(errnobuf), "%d", errno); + cbm_log_warn("compat.popen_isolated_failed", "stage", stage, "gle", glebuf, "errno", + errnobuf); + } + return fp; + } + g_popen_last_isolated = 0; return _popen(cmd, mode); } int cbm_pclose(FILE *f) { - return _pclose(f); + InitOnceExecuteOnce(&g_popen_once, cbm_popen_init, NULL, NULL); + + HANDLE proc = NULL; + EnterCriticalSection(&g_popen_lock); + for (int i = 0; i < CBM_POPEN_MAX; i++) { + if (g_popen_tab[i].fp == f) { + proc = g_popen_tab[i].proc; + g_popen_tab[i].fp = NULL; + g_popen_tab[i].proc = NULL; + break; + } + } + LeaveCriticalSection(&g_popen_lock); + + if (!proc) { + return _pclose(f); /* opened via _popen (non-read mode) */ + } + fclose(f); + WaitForSingleObject(proc, INFINITE); + DWORD code = 0; + BOOL got = GetExitCodeProcess(proc, &code); + CloseHandle(proc); + return got ? (int)code : -1; } FILE *cbm_fopen(const char *path, const char *mode) { diff --git a/src/foundation/compat_fs_internal.h b/src/foundation/compat_fs_internal.h index 47ad5309..ba0f89ba 100644 --- a/src/foundation/compat_fs_internal.h +++ b/src/foundation/compat_fs_internal.h @@ -31,6 +31,16 @@ */ wchar_t *cbm_build_cmdline(const char *const *argv); +/* + * Test hook for the isolated popen path (#798): returns 1 when the most + * recent cbm_popen(..., "r") stream was produced by the isolated + * CreateProcessW + PROC_THREAD_ATTRIBUTE_HANDLE_LIST spawn, 0 otherwise + * (e.g. a non-read mode routed to _popen, or a failed isolated spawn). + * Not synchronized across threads; intended for single-threaded test + * assertions only. + */ +int cbm_popen_last_was_isolated(void); + #endif /* _WIN32 */ #endif /* CBM_FOUNDATION_COMPAT_FS_INTERNAL_H */ diff --git a/tests/test_security.c b/tests/test_security.c index 202e4301..5e05be2a 100644 --- a/tests/test_security.c +++ b/tests/test_security.c @@ -517,6 +517,47 @@ TEST(exec_no_shell_win_null_argv_returns_error) { PASS(); } +/* ────────────────────────────────────────────────────────────────── + * WINDOWS ISOLATED POPEN (handle-inheritance fix for #798) + * + * Regression guard for the UI hang: _popen spawns children with + * bInheritHandles=TRUE, leaking every inheritable handle (listening + * sockets, Winsock/AFD helpers) into git-for-Windows, whose MSYS2 + * runtime hangs classifying them via NtQueryObject. cbm_popen must + * instead spawn via CreateProcessW + PROC_THREAD_ATTRIBUTE_HANDLE_LIST. + * + * On windows-latest CI (real git-for-Windows) these prove: + * - the returned stream came from the isolated spawn, not _popen + * (cbm_popen_last_was_isolated test hook) — a revert to raw _popen + * turns the hook 0 and fails the guard; + * - stdout and the child exit code round-trip through + * cbm_popen/cbm_pclose. + * NOT proven here: the full UI repro (listening socket + MSYS2 handle + * walk under a single-threaded server) — follow-up harness. + * ────────────────────────────────────────────────────────────────── */ + +TEST(popen_isolated_git_version_round_trip) { + FILE *fp = cbm_popen("git --version", "r"); + ASSERT_NOT_NULL(fp); + ASSERT_EQ(cbm_popen_last_was_isolated(), 1); + char line[256]; + line[0] = '\0'; + ASSERT_NOT_NULL(fgets(line, sizeof(line), fp)); + ASSERT(strncmp(line, "git version", strlen("git version")) == 0); + ASSERT_EQ(cbm_pclose(fp), 0); + PASS(); +} + +TEST(popen_isolated_propagates_exit_code) { + /* Runs under `cmd.exe /c`, so a bare `exit 7` is the child exit code; + * cbm_pclose must surface it via GetExitCodeProcess. */ + FILE *fp = cbm_popen("exit 7", "r"); + ASSERT_NOT_NULL(fp); + ASSERT_EQ(cbm_popen_last_was_isolated(), 1); + ASSERT_EQ(cbm_pclose(fp), 7); + PASS(); +} + #endif /* _WIN32 */ /* ══════════════════════════════════════════════════════════════════ @@ -585,5 +626,8 @@ SUITE(security) { RUN_TEST(exec_no_shell_win_exit_zero); RUN_TEST(exec_no_shell_win_captures_exit_code); RUN_TEST(exec_no_shell_win_null_argv_returns_error); + /* Isolated popen — handle-inheritance regression guard for #798 */ + RUN_TEST(popen_isolated_git_version_round_trip); + RUN_TEST(popen_isolated_propagates_exit_code); #endif }