diff --git a/src/foundation/compat_fs.c b/src/foundation/compat_fs.c index 886f5c459..358986453 100644 --- a/src/foundation/compat_fs.c +++ b/src/foundation/compat_fs.c @@ -20,7 +20,9 @@ #endif #include #include /* _wmkdir */ -#include /* _wunlink */ +#include /* _O_RDONLY */ +#include /* _wunlink, _open_osfhandle, _close */ +#include /* intptr_t */ #include "foundation/win_utf8.h" struct cbm_dir { @@ -122,12 +124,170 @@ 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 CreateProcess with STARTUPINFOEX + 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. */ + +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; + +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; +} + +static FILE *cbm_popen_isolated(const char *cmd) { + 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)) { + 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. */ + HANDLE nul = CreateFileA("NUL", GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, OPEN_EXISTING, 0, NULL); + + HANDLE inherit[2]; + DWORD ninherit = 0; + inherit[ninherit++] = wr; + if (nul != INVALID_HANDLE_VALUE) { + inherit[ninherit++] = 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 prepared = attr && InitializeProcThreadAttributeList(attr, 1, 0, &attr_sz) && + UpdateProcThreadAttribute(attr, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherit, + ninherit * sizeof(HANDLE), NULL, NULL); + + STARTUPINFOEXA 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. */ + char cmdline[2048]; + int n = snprintf(cmdline, sizeof(cmdline), "cmd.exe /c %s", cmd); + + PROCESS_INFORMATION pi; + ZeroMemory(&pi, sizeof(pi)); + BOOL created = prepared && n > 0 && n < (int)sizeof(cmdline) && + CreateProcessA(NULL, cmdline, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, &si.StartupInfo, &pi); + + if (attr) { + DeleteProcThreadAttributeList(attr); + free(attr); + } + CloseHandle(wr); /* the child owns the write-end now */ + if (nul != INVALID_HANDLE_VALUE) { + CloseHandle(nul); + } + if (!created) { + CloseHandle(rd); + return NULL; + } + CloseHandle(pi.hThread); + + int fd = _open_osfhandle((intptr_t)rd, _O_RDONLY); + if (fd == -1) { + CloseHandle(rd); + CloseHandle(pi.hProcess); + return NULL; + } + FILE *fp = _fdopen(fd, "r"); /* takes ownership of fd/rd */ + if (!fp) { + _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. */ + CloseHandle(pi.hProcess); + fclose(fp); + return NULL; +} + FILE *cbm_popen(const char *cmd, const char *mode) { + /* Our git shell-outs are all read-mode; only those need the isolation. */ + if (mode && mode[0] == 'r' && mode[1] == '\0') { + FILE *fp = cbm_popen_isolated(cmd); + if (fp) { + return fp; + } + /* fall through to _popen if isolated spawn failed for any reason */ + } 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 the _popen fallback */ + } + fclose(f); + WaitForSingleObject(proc, INFINITE); + DWORD code = 0; + GetExitCodeProcess(proc, &code); + CloseHandle(proc); + return (int)code; } FILE *cbm_fopen(const char *path, const char *mode) { diff --git a/tests/test_httpd.c b/tests/test_httpd.c index ddbca5568..cf16a025f 100644 --- a/tests/test_httpd.c +++ b/tests/test_httpd.c @@ -17,6 +17,7 @@ #include "../src/foundation/platform.h" #include "../src/cli/cli.h" #include "../src/ui/http_server.h" +#include "git/git_context.h" #include "test_framework.h" #include "test_helpers.h" #include "ui/httpd.h" @@ -419,6 +420,51 @@ static void th_server_stop(th_server_t *ts) { cbm_http_server_free(ts->srv); } +/* ── Watchdog: turn a hang into a hard failure ─────────────────── */ + +/* A deadlocked git child cannot be recovered in-process, so a "must not hang" + * guard needs a watchdog thread that force-fails the whole run if the operation + * under test does not signal completion within a generous window. On a healthy + * build the flag is set long before the window elapses and the watchdog is a + * no-op. */ +typedef struct { + volatile int done; + int timeout_ms; + const char *what; +} th_watchdog_t; + +static void *th_watchdog_fn(void *arg) { + th_watchdog_t *wd = (th_watchdog_t *)arg; + int waited = 0; + while (waited < wd->timeout_ms) { + cbm_usleep(100 * 1000); + waited += 100; + if (wd->done) + return NULL; + } + if (!wd->done) { + fprintf(stderr, + "\n FATAL: watchdog fired after %dms — %s hung " + "(handle-inheritance deadlock, issue #798)\n", + wd->timeout_ms, wd->what); + fflush(stderr); + _exit(99); /* _exit, not exit: don't run atexit handlers from a wedged process */ + } + return NULL; +} + +/* Probe for a usable git on PATH. Called BEFORE the UI server starts (no socket + * handles exist yet), so this cbm_popen can never trip the #798 deadlock. */ +static int th_git_available(void) { + FILE *fp = cbm_popen("git --version", "r"); + if (!fp) + return 0; + char buf[128]; + char *got = fgets(buf, sizeof(buf), fp); + cbm_pclose(fp); + return got && strncmp(buf, "git version", 11) == 0; +} + TEST(ui_server_unknown_path_404) { th_server_t ts; ASSERT_EQ(th_server_start(&ts), 0); @@ -682,6 +728,190 @@ TEST(ui_server_stop_joins_cleanly) { PASS(); } +/* ── Regression: #798 — cbm_popen must not leak handles into the git child ── + * + * Root cause: on Windows the UI HTTP server calls WSAStartup and holds + * listening/AFD socket handles. list_projects resolves git context per project + * (add_git_context_json → cbm_git_context_resolve → git_capture → cbm_popen). + * The CRT _popen spawns via CreateProcess(bInheritHandles=TRUE), leaking EVERY + * inheritable handle — including those AFD handles — into git-for-Windows, whose + * MSYS/Cygwin startup classifies each inherited handle with NtQueryObject and + * DEADLOCKS on a socket/AFD handle. The single-threaded UI server then wedges + * and the web UI hangs forever. The fix reimplements cbm_popen to inherit ONLY + * the stdout pipe (STARTUPINFOEX + PROC_THREAD_ATTRIBUTE_HANDLE_LIST). + * + * Guard strategy — two layers, because the *deadlock* is environment-sensitive: + * whether NtQueryObject actually hangs depends on the git-for-Windows/MSYS + * runtime and the AFD provider, so a git-based test cannot be relied on to go + * RED everywhere (verified: it does NOT reproduce on every dev machine, and the + * Linux CI has no such sockets at all). So the load-bearing guard tests the + * fix's *contract* directly and deterministically — + * cbm_popen_isolates_inheritable_handle below asserts no foreign inheritable + * handle crosses into the child, which goes RED on any Windows without the fix. + * + * The two tests here are end-to-end liveness invariants: they enforce "git + * context resolves / list_projects responds while the UI server holds live + * sockets, under a watchdog." They pass with or without the fix on git builds + * that don't exhibit the NtQueryObject hang, but they catch #798 on the affected + * builds and guard against any future hang regression in this path. (Same + * pattern as the environment-sensitive guards in test_git_context.c.) + * + * A real repo is unnecessary — resolving any existing directory spawns + * `git -C rev-parse …`, exercising the exact cbm_popen shell-out; + * that also sidesteps the Windows CI shell's inability to `git init` via + * system(). On POSIX popen() already sets O_CLOEXEC, so this is a sanity check + * there. */ +TEST(ui_server_git_context_under_live_sockets_no_hang) { + /* Probe git before any socket exists — safe, cannot deadlock. */ + if (!th_git_available()) + SKIP_PLATFORM("git not on PATH — cannot exercise the git shell-out"); + + char *tmp = th_mktempdir("cbm_ui798"); + if (!tmp) + FAIL("th_mktempdir returned NULL"); + + /* Bring up the real UI server — this allocates the AFD/socket handles that + * the unfixed cbm_popen would leak into git. */ + th_server_t ts; + if (th_server_start(&ts) != 0) { + th_rmtree(tmp); + FAIL("th_server_start failed"); + } + + th_watchdog_t wd = {0, 30000, "cbm_git_context_resolve under live UI sockets"}; + cbm_thread_t wdt; + if (cbm_thread_create(&wdt, 0, th_watchdog_fn, &wd) != 0) { + th_server_stop(&ts); + th_rmtree(tmp); + FAIL("watchdog thread create failed"); + } + + /* The exact per-project resolve list_projects runs. Without the fix this + * spawns git with the AFD handles inherited → NtQueryObject deadlock → never + * returns → watchdog force-fails the process. */ + cbm_git_context_t ctx = {0}; + int rc = cbm_git_context_resolve(tmp, &ctx); + + wd.done = 1; /* disarm before the watchdog window elapses */ + cbm_thread_join(&wdt); + + int returned = (rc == 0); /* resolve reached its return without hanging */ + cbm_git_context_free(&ctx); + th_server_stop(&ts); + th_rmtree(tmp); + + ASSERT_EQ(returned, 1); + PASS(); +} + +/* End-to-end companion: the actual endpoint Martin named must answer under UI + * mode. Drives POST /rpc list_projects against the live server under a watchdog + * and asserts a 200 within the window. With projects present in the store this + * also walks the per-project git shell-out end-to-end; with an empty store it + * still proves the endpoint stays responsive under UI mode. */ +TEST(ui_server_rpc_list_projects_responds) { + th_server_t ts; + ASSERT_EQ(th_server_start(&ts), 0); + int port = cbm_http_server_port(ts.srv); + + th_watchdog_t wd = {0, 30000, "POST /rpc list_projects"}; + cbm_thread_t wdt; + ASSERT_EQ(cbm_thread_create(&wdt, 0, th_watchdog_fn, &wd), 0); + + const char *body = "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"list_projects\",\"arguments\":{}}}"; + char req[512]; + snprintf(req, sizeof(req), + "POST /rpc HTTP/1.1\r\n" + "Content-Type: application/json\r\n" + "Content-Length: %d\r\n\r\n%s", + (int)strlen(body), body); + char resp[8192]; + int n = th_http(port, req, resp, sizeof(resp)); + + wd.done = 1; + cbm_thread_join(&wdt); + th_server_stop(&ts); + + ASSERT_GT(n, 0); + ASSERT_EQ(th_status(resp), 200); + ASSERT_NOT_NULL(strstr(resp, "\"jsonrpc\"")); + PASS(); +} + +/* Deterministic contract guard for the #798 fix: cbm_popen must NOT let a + * foreign inheritable handle cross into the child it spawns. This is the exact + * property whose absence caused the deadlock (git inheriting an AFD handle), but + * it is tested directly — independent of whether the local git-for-Windows build + * actually hangs on NtQueryObject — so it goes RED on any Windows build without + * the fix and GREEN with it. + * + * Mechanism: create a marker file, opened inheritable, holding a known 16-byte + * magic. Spawn — through cbm_popen, i.e. the real fixed spawn path — a re-exec + * of this test binary in its hidden __handle_probe mode. Windows preserves + * handle values across inheritance, so the child can test whether the parent's + * handle value is a valid, readable copy of the marker. Without the fix _popen + * leaks the marker (via cmd.exe) into the probe → it reads the magic → exit 1. + * With the fix only the stdout pipe is inherited → the probe cannot read it → + * exit 0. cbm_pclose surfaces that exit code. */ +TEST(cbm_popen_isolates_inheritable_handle) { +#ifndef _WIN32 + SKIP_PLATFORM("cbm_popen handle isolation is Windows-specific (POSIX popen uses O_CLOEXEC)"); +#else + char self[1024]; + if (GetModuleFileNameA(NULL, self, sizeof(self)) == 0) + FAIL("GetModuleFileNameA failed"); + + char *tmp = th_mktempdir("cbm_leak798"); + if (!tmp) + FAIL("th_mktempdir returned NULL"); + char marker[1024]; + snprintf(marker, sizeof(marker), "%s\\marker.bin", tmp); + + static const unsigned char MAGIC[16] = {0xC0, 0xFF, 0xEE, 0x42, 0xDE, 0xAD, 0xBE, 0xEF, + 0x13, 0x37, 0xA5, 0x5A, 0x0B, 0xAD, 0xF0, 0x0D}; + char magic_hex[33]; + for (int i = 0; i < 16; i++) + snprintf(magic_hex + i * 2, 3, "%02x", MAGIC[i]); + + /* Inheritable so the UNFIXED _popen would leak it; the fix must exclude it. */ + SECURITY_ATTRIBUTES sa = {sizeof(sa), NULL, TRUE}; + HANDLE h = CreateFileA(marker, GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, NULL); + if (h == INVALID_HANDLE_VALUE) { + th_rmtree(tmp); + FAIL("CreateFileA marker failed"); + } + DWORD wrote = 0; + WriteFile(h, MAGIC, 16, &wrote, NULL); + FlushFileBuffers(h); + + /* Double-outer-quote form so cmd.exe parses a possibly-spaced exe path. */ + char cmd[2048]; + snprintf(cmd, sizeof(cmd), "\"\"%s\" __handle_probe %lld %s\"", self, + (long long)(intptr_t)h, magic_hex); + + FILE *fp = cbm_popen(cmd, "r"); + if (!fp) { + CloseHandle(h); + th_rmtree(tmp); + FAIL("cbm_popen returned NULL"); + } + char sink[256]; + while (fgets(sink, sizeof(sink), fp)) { + /* probe writes nothing to stdout; drain defensively */ + } + int leaked = cbm_pclose(fp); /* 1 iff the probe could read our marker */ + + CloseHandle(h); + th_rmtree(tmp); + + ASSERT_EQ(leaked, 0); + PASS(); +#endif +} + /* ── Suite ────────────────────────────────────────────────────── */ SUITE(httpd) { @@ -722,4 +952,9 @@ SUITE(httpd) { RUN_TEST(ui_server_slow_request_hits_deadline); RUN_TEST(ui_server_access_log_redacts_query); RUN_TEST(ui_server_stop_joins_cleanly); + + /* Regression #798: cbm_popen must not leak handles into the git child */ + RUN_TEST(cbm_popen_isolates_inheritable_handle); + RUN_TEST(ui_server_git_context_under_live_sockets_no_hang); + RUN_TEST(ui_server_rpc_list_projects_responds); } diff --git a/tests/test_main.c b/tests/test_main.c index 67d3d81b3..f809e59e3 100644 --- a/tests/test_main.c +++ b/tests/test_main.c @@ -13,6 +13,40 @@ int tf_skip_count = 0; #include #include +#ifdef _WIN32 +#include +#include +#include +/* Hidden re-exec mode for the cbm_popen handle-isolation regression test + * (tests/test_httpd.c). Invoked as: test-runner __handle_probe . + * Reports, via exit code, whether a marker file handle created inheritable by + * the parent was actually inherited into this (grand)child: + * exit 1 = marker handle is readable here → it LEAKED across the spawn + * exit 0 = marker handle is not present → spawn correctly isolated it + * Windows preserves handle values across inheritance, so the parent's handle + * value refers to the same object here iff it was inherited. */ +static int th_handle_probe(int argc, char **argv) { + if (argc < 4) { + return 0; + } + HANDLE h = (HANDLE)(intptr_t)_strtoi64(argv[2], NULL, 10); + unsigned char want[16], got[16]; + for (int i = 0; i < 16; i++) { + unsigned v = 0; + if (sscanf(argv[3] + i * 2, "%2x", &v) != 1) { + return 0; + } + want[i] = (unsigned char)v; + } + SetFilePointer(h, 0, NULL, FILE_BEGIN); + DWORD nread = 0; + if (ReadFile(h, got, 16, &nread, NULL) && nread == 16 && memcmp(got, want, 16) == 0) { + return 1; /* marker readable → the handle was leaked into this child */ + } + return 0; +} +#endif + static int g_suite_argc = 0; static char **g_suite_argv = NULL; @@ -133,6 +167,12 @@ extern void suite_dump_verify_io(void); extern void cbm_kind_in_set_free_cache(void); int main(int argc, char **argv) { +#ifdef _WIN32 + /* Hidden probe mode — must run before any suite output (see th_handle_probe). */ + if (argc >= 2 && strcmp(argv[1], "__handle_probe") == 0) { + return th_handle_probe(argc, argv); + } +#endif g_suite_argc = argc; g_suite_argv = argv; printf("\n codebase-memory-mcp C test suite\n");