Merge pull request #846 from DeusData/distill/799-win-handles

fix(windows): spawn git without inheriting handles (UI hang)

Author: DeusData

src/foundation/compat_fs.c

@@ -21,7 +21,11 @@
 #endif
 #include <windows.h>
 #include <direct.h> /* _wmkdir */
-#include <io.h>     /* _wunlink */
+#include <errno.h>  /* errno for spawn-failure logging */
+#include <fcntl.h>  /* _O_RDONLY */
+#include <io.h>     /* _wunlink, _open_osfhandle, _close */
+#include <stdint.h> /* 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 <system dir>\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) {

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 */

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
 }