From 982916b0f2a58f9f98409f289cedec6c03240e91 Mon Sep 17 00:00:00 2001 From: Flipper Date: Sun, 5 Jul 2026 13:06:15 +0200 Subject: [PATCH] fix(win): escape spawned command-line args so the index worker gets valid argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the Windows-only smoke-windows (standard + ui) failure in the release dry run, where index_repository reported outcome=exit_nonzero and "Indexing worker crashed on a file" on big_templated.hpp. It is not the #424 allocator class and not big_templated.hpp — the file parses fine; the worker never reaches it. This is the Windows residual of #838. That issue fixed the CLI parser to accept the supervisor's raw-JSON-positional + --response-out flag layout, verified with a PowerShell repro where the JSON argument arrives intact. But the real supervisor spawns the worker via CreateProcessA, whose command-line re-parse strips the JSON's quotes, so on Windows the now-fixed parser still receives corrupted JSON. On Windows the index supervisor spawns the worker as ` cli --index-worker index_repository ...`, but cbm_run_win built the CreateProcess command line by wrapping each argv element in bare quotes with no escaping. Windows re-parses that single string back into argv and strips the inner quotes, so the JSON argument {"repo_path":"..."} arrived at the child as {repo_path:...} — invalid JSON. The worker exited non-zero at arg parse ("repo_path is required"); the supervisor classified exit_nonzero and blamed the last-marked file (big_templated.hpp), producing the misleading crash message. POSIX is unaffected: cbm_run_posix passes the argv array straight to execv with no re-parse, which is why only smoke-windows was red while Linux/macOS indexed the same fixture fine. Fix: cbm_build_win_cmdline() applies the Microsoft C runtime quoting rules (quote-wrap + escape embedded quotes and their preceding backslash runs) so the child re-parses byte-identical argv. It is defined unconditionally (pure string logic, no Windows headers) and unit-tested on every platform via a round-trip against a reference CommandLineToArgvW parser. Also in this change: - ui/http_server.c: the UI index spawn had a byte-for-byte duplicate of the same bug (its own "%s" cmdline). Route it through cbm_build_win_cmdline too. - mcp/index_supervisor.c: log the worker's raw exit_code and KEEP its log on any failure (delete only on a clean run). Previously the log was always deleted and only outcome+signal were logged, so a non-zero worker left nothing to diagnose — the CI blind spot that hid this bug behind a generic message. A separate hardening of pipeline/artifact.c's single-quoted git shell-outs (a different bug class — popen command-string quoting, not CreateProcess argv) is split into a follow-up PR with its own guard test, per review. Verified on a CLANG64 build: the supervised big_templated.hpp fixture now indexes clean (reap outcome=clean exit_code=0); subprocess suite 11/11, affected suites (git_context, index_resilience, ui, httpd, mcp, security, str_util) green. Refs: #838, #423 Signed-off-by: Flipper --- src/foundation/subprocess.c | 104 +++++++++++++++++++++++++---- src/foundation/subprocess.h | 14 ++++ src/mcp/index_supervisor.c | 26 +++++++- src/ui/http_server.c | 18 +++-- tests/test_subprocess.c | 128 ++++++++++++++++++++++++++++++++++++ 5 files changed, 271 insertions(+), 19 deletions(-) diff --git a/src/foundation/subprocess.c b/src/foundation/subprocess.c index 2b248794..61730c24 100644 --- a/src/foundation/subprocess.c +++ b/src/foundation/subprocess.c @@ -132,6 +132,94 @@ static bool cbm_tail_log(const char *log_file, long *tail_pos, cbm_proc_log_cb c return progressed; } +/* ── Windows command-line quoting (pure; unit-tested on every platform) ─────── */ + +/* Append char `c` to buf[cap], reserving the final byte for a NUL terminator. + * Sets *ovf on overflow and stops writing; pos keeps advancing so the caller + * still detects the overflow after the loop. */ +static size_t cbm_cmdline_put(char *buf, size_t cap, size_t pos, char c, bool *ovf) { + if (pos + 1 >= cap) { + *ovf = true; + return pos; + } + buf[pos] = c; + return pos + 1; +} + +/* Append one argv element to the command line using the Microsoft C runtime + * quoting rules (see MS "Parsing C Command-Line Arguments"). CreateProcess takes + * a SINGLE string that the child re-parses back into argv, so any element with a + * space, tab or double-quote must be wrapped in quotes and its embedded quotes / + * preceding backslashes escaped. Without this a JSON argument like + * {"repo_path":"C:/r"} loses its inner quotes and the child receives the invalid + * {repo_path:C:/r} — the Windows-only index-worker cmdline-quoting bug (the worker exited + * non-zero at JSON-arg parse, misattributed to the last-marked file). POSIX is + * unaffected: cbm_run_posix passes the argv array straight to execv. */ +static size_t cbm_cmdline_append_arg(char *buf, size_t cap, size_t pos, const char *arg, bool first, + bool *ovf) { + if (!first) { + pos = cbm_cmdline_put(buf, cap, pos, ' ', ovf); + } + pos = cbm_cmdline_put(buf, cap, pos, '"', ovf); + for (const char *p = arg; *p;) { + size_t nbs = 0; + while (*p == '\\') { + nbs++; + p++; + } + if (*p == '\0') { + /* Trailing backslashes precede the closing quote: double them so the + * quote stays a delimiter, not an escaped literal. */ + for (size_t k = 0; k < nbs * 2; k++) { + pos = cbm_cmdline_put(buf, cap, pos, '\\', ovf); + } + break; + } + if (*p == '"') { + /* N backslashes then a quote -> 2N+1 backslashes then an escaped quote. */ + for (size_t k = 0; k < nbs * 2 + 1; k++) { + pos = cbm_cmdline_put(buf, cap, pos, '\\', ovf); + } + pos = cbm_cmdline_put(buf, cap, pos, '"', ovf); + p++; + } else { + for (size_t k = 0; k < nbs; k++) { + pos = cbm_cmdline_put(buf, cap, pos, '\\', ovf); + } + pos = cbm_cmdline_put(buf, cap, pos, *p, ovf); + p++; + } + } + pos = cbm_cmdline_put(buf, cap, pos, '"', ovf); + return pos; +} + +/* Build a full Windows CreateProcess command line from a NULL-terminated argv, + * applying the MS C runtime quoting rules so the child re-parses byte-identical + * argv. Returns true on success, false if the result would overflow `buf`. + * + * Defined unconditionally (pure string logic, no Windows headers) so the quoting + * contract is unit-tested on Linux/macOS CI too — even though the real spawn path + * only runs on Windows. Shared by cbm_run_win AND the UI http_server index spawn + * so both escape identically; a naive `"%s"` wrap silently corrupts any argument + * containing a quote (e.g. the index JSON {"repo_path":"…"}), corrupting the + * spawned child's argv. */ +bool cbm_build_win_cmdline(char *buf, size_t cap, const char *const *argv) { + if (!buf || cap == 0 || !argv) { + return false; + } + size_t pos = 0; + bool ovf = false; + for (int i = 0; argv[i]; i++) { + pos = cbm_cmdline_append_arg(buf, cap, pos, argv[i], i == 0, &ovf); + if (ovf) { + return false; + } + } + buf[pos] = '\0'; + return true; +} + #ifdef _WIN32 static int cbm_run_win(const cbm_proc_opts_t *opts, cbm_proc_result_t *out) { @@ -139,18 +227,12 @@ static int cbm_run_win(const cbm_proc_opts_t *opts, cbm_proc_result_t *out) { const char *const default_argv[] = {bin, NULL}; const char *const *argv = opts->argv ? opts->argv : default_argv; - /* Build a quoted command line from argv. */ char cmdline[8192]; - size_t pos = 0; - for (int i = 0; argv[i]; i++) { - int n = snprintf(cmdline + pos, sizeof(cmdline) - pos, "%s\"%s\"", (i ? " " : ""), argv[i]); - if (n < 0 || (size_t)n >= sizeof(cmdline) - pos) { - out->outcome = CBM_PROC_SPAWN_FAILED; - out->exit_code = -1; - out->term_signal = 0; - return -1; - } - pos += (size_t)n; + if (!cbm_build_win_cmdline(cmdline, sizeof(cmdline), argv)) { + out->outcome = CBM_PROC_SPAWN_FAILED; + out->exit_code = -1; + out->term_signal = 0; + return -1; } HANDLE hlog = INVALID_HANDLE_VALUE; diff --git a/src/foundation/subprocess.h b/src/foundation/subprocess.h index bb8d683a..826055b5 100644 --- a/src/foundation/subprocess.h +++ b/src/foundation/subprocess.h @@ -21,6 +21,7 @@ #define CBM_SUBPROCESS_H #include +#include /* size_t (cbm_build_win_cmdline) */ /* How a supervised child ended. */ typedef enum { @@ -73,4 +74,17 @@ cbm_proc_outcome_t cbm_proc_classify(bool exited_normally, int exit_code, int te /* Stable lowercase name for an outcome (for structured logs / skip reasons). */ const char *cbm_proc_outcome_str(cbm_proc_outcome_t o); +/* Build a Windows CreateProcess command line from a NULL-terminated argv, applying + * the Microsoft C runtime quoting rules (quote-wrap + escape embedded quotes and + * their preceding backslashes) so the spawned child re-parses byte-identical argv. + * Returns true on success, false on overflow (buf then holds a truncated string). + * + * CreateProcess re-parses a SINGLE command string into argv, so a naive `"%s"` wrap + * silently corrupts any element containing a double-quote — e.g. the index worker's + * JSON arg {"repo_path":"…"} arrives as {repo_path:…}, the Windows index-worker bug. + * Exposed (and compiled on every platform — it is pure string logic) so the quoting + * is unit-tested on Linux/macOS CI, and so both spawn sites (cbm_subprocess_run and + * the UI http_server index spawn) escape through one shared, tested implementation. */ +bool cbm_build_win_cmdline(char *buf, size_t cap, const char *const *argv); + #endif /* CBM_SUBPROCESS_H */ diff --git a/src/mcp/index_supervisor.c b/src/mcp/index_supervisor.c index 45a56b8f..6fb99b9a 100644 --- a/src/mcp/index_supervisor.c +++ b/src/mcp/index_supervisor.c @@ -192,7 +192,10 @@ int cbm_index_spawn_worker(const char *args_json, bool single_thread, const char opts.argv = argv; opts.log_file = log_path; opts.quiet_timeout_ms = worker_quiet_timeout_ms(); - opts.delete_log_on_exit = true; + /* We manage log deletion ourselves after reaping (below): keep it on failure + * for post-mortem, delete it only on a clean run. See the observability + * note at the reap site. */ + opts.delete_log_on_exit = false; cbm_proc_result_t r; int run_rc = cbm_subprocess_run(&opts, &r); @@ -209,6 +212,7 @@ int cbm_index_spawn_worker(const char *args_json, bool single_thread, const char if (run_rc != 0) { (void)remove(resp_path); + (void)remove(log_path); /* empty/partial log from a failed spawn — nothing to keep */ cbm_log_warn("index.supervisor.spawn_failed", "action", "degrade_in_process"); return -1; } @@ -222,9 +226,25 @@ int cbm_index_spawn_worker(const char *args_json, bool single_thread, const char (void)remove(resp_path); char sig[16]; + char exit_buf[16]; snprintf(sig, sizeof(sig), "%d", r.term_signal); - cbm_log_info("index.supervisor.reap", "outcome", cbm_proc_outcome_str(r.outcome), "signal", - sig); + snprintf(exit_buf, sizeof(exit_buf), "%d", r.exit_code); + cbm_log_info("index.supervisor.reap", "outcome", cbm_proc_outcome_str(r.outcome), "exit_code", + exit_buf, "signal", sig); + + /* Observability: on a CLEAN run the worker log is noise → delete it. On + * ANY failure keep it and surface its path + raw exit code, so the worker's own + * stdout/stderr (pipeline logs, any assert/abort text, the exact exit code) is + * available post-mortem instead of vanishing. Previously the log was ALWAYS + * deleted and only outcome+signal were logged, so a worker that exited non-zero + * left nothing to diagnose — the CI blind spot that hid this bug (a mangled JSON + * arg → "repo_path is required" exit) behind a generic "crashed on a file". */ + if (r.outcome == CBM_PROC_CLEAN) { + (void)remove(log_path); + } else { + cbm_log_warn("index.supervisor.worker_failed", "outcome", cbm_proc_outcome_str(r.outcome), + "exit_code", exit_buf, "log", log_path); + } return 0; } diff --git a/src/ui/http_server.c b/src/ui/http_server.c index 578ccd4d..aef72aef 100644 --- a/src/ui/http_server.c +++ b/src/ui/http_server.c @@ -33,6 +33,7 @@ #include "foundation/compat_fs.h" #include "foundation/str_util.h" #include "foundation/compat_thread.h" +#include "foundation/subprocess.h" /* cbm_build_win_cmdline — shared MS-CRT arg quoting */ #include #include @@ -957,13 +958,20 @@ static void *index_thread_fn(void *arg) { snprintf(log_file, sizeof(log_file), "%s\\cbm_index_%d.log", getenv("TEMP") ? getenv("TEMP") : ".", (int)_getpid()); - /* Build command line for CreateProcess */ - char cmdline[2048]; - /* --index-worker: this http_server spawn is already the crash-isolation layer, + /* Build command line for CreateProcess through the shared MS-CRT quoter so the + * JSON arg's embedded quotes survive the child's argv re-parse — a naive + * `"%s"` wrap dropped them, corrupting {"repo_path":"…"} into {repo_path:…}. + * --index-worker: this http_server spawn is already the crash-isolation layer, * so the child runs indexing in-process rather than spawning its own supervisor * (avoids redundant process nesting). */ - snprintf(cmdline, sizeof(cmdline), "\"%s\" cli --index-worker index_repository \"%s\"", bin, - json_arg); + char cmdline[2048]; + const char *const idx_argv[] = {bin, "cli", "--index-worker", "index_repository", + json_arg, NULL}; + if (!cbm_build_win_cmdline(cmdline, sizeof(cmdline), idx_argv)) { + snprintf(job->error_msg, sizeof(job->error_msg), "index command line too long"); + atomic_store(&job->status, 3); + return NULL; + } cbm_log_info("ui.index.spawn", "bin", bin, "log", log_file); diff --git a/tests/test_subprocess.c b/tests/test_subprocess.c index bd73e7ed..d4138187 100644 --- a/tests/test_subprocess.c +++ b/tests/test_subprocess.c @@ -168,6 +168,131 @@ TEST(subprocess_run_null_bin_rejected) { PASS(); } +/* ── Layer 3: Windows command-line quoting (pure; every platform) ───────────── + * + * The Windows index-worker "crash" was a quoting bug: the Windows spawn wrapped each + * argv element in bare quotes without escaping, so a JSON argument like + * {"repo_path":"C:/r"} lost its inner quotes when the child re-parsed the command + * line — the worker then failed at JSON-arg parse and exited non-zero, which the + * supervisor misreported as a per-file crash. We guard cbm_build_win_cmdline by + * ROUND-TRIP: a reference implementation of the Windows CommandLineToArgvW rules + * (the inverse of the builder) must re-parse the emitted line back into the exact + * original argv. Testing the invariant — not a hand-computed escaped string — keeps + * the guard honest and readable, and runs on Linux/macOS CI (the builder is pure). */ + +/* Reference re-parser: the subset of CommandLineToArgvW our builder emits (every + * arg quote-wrapped; \" for embedded quotes; backslashes doubled before a quote). */ +static int parse_win_cmdline(const char *cmd, char out[][256], int max_args) { + int argc = 0; + const char *p = cmd; + while (*p) { + while (*p == ' ' || *p == '\t') { + p++; + } + if (!*p || argc >= max_args) { + break; + } + char *o = out[argc]; + size_t oi = 0; + bool in_quotes = false; + for (;;) { + size_t nbs = 0; + while (*p == '\\') { + nbs++; + p++; + } + if (*p == '"') { + for (size_t k = 0; k < nbs / 2; k++) { + o[oi++] = '\\'; + } + if (nbs % 2) { + o[oi++] = '"'; /* odd run → the quote is an escaped literal */ + } else { + in_quotes = !in_quotes; /* even run → the quote is a delimiter */ + } + p++; + } else { + for (size_t k = 0; k < nbs; k++) { + o[oi++] = '\\'; + } + if (*p == '\0' || (!in_quotes && (*p == ' ' || *p == '\t'))) { + break; + } + o[oi++] = *p++; + } + } + o[oi] = '\0'; + argc++; + } + return argc; +} + +static bool cmdline_roundtrips(const char *const *argv) { + char cmd[4096]; + if (!cbm_build_win_cmdline(cmd, sizeof(cmd), argv)) { + return false; + } + char parsed[16][256]; + int pc = parse_win_cmdline(cmd, parsed, 16); + int oc = 0; + while (argv[oc]) { + oc++; + } + if (pc != oc) { + return false; + } + for (int i = 0; i < oc; i++) { + if (strcmp(argv[i], parsed[i]) != 0) { + return false; + } + } + return true; +} + +/* The exact index-worker argv: the command line with a JSON arg full of quotes. + * Round-trips, AND the emitted line must contain an ESCAPED quote (\") — the bare + * `"%s"` wrap that caused the bug never would. */ +TEST(win_cmdline_index_worker_json) { + const char *const argv[] = {"C:/bin/cbm.exe", "cli", + "--index-worker", "index_repository", + "{\"repo_path\":\"C:/r\"}", "--response-out", + "C:/c/w.response", NULL}; + ASSERT(cmdline_roundtrips(argv)); + char cmd[4096]; + ASSERT(cbm_build_win_cmdline(cmd, sizeof(cmd), argv)); + ASSERT(strstr(cmd, "\\\"repo_path\\\"") != NULL); /* inner quotes are escaped */ + PASS(); +} + +/* A battery of adversarial argv (spaces, tabs, embedded quotes, backslash runs, + * trailing backslashes, backslash-before-quote, real Windows paths) must all + * round-trip byte-for-byte through the builder + reference parser. */ +TEST(win_cmdline_roundtrip_battery) { + const char *const a1[] = {"a", "b", NULL}; + const char *const a2[] = {"has space", "tab\there", NULL}; + const char *const a3[] = {"trailing\\", "a\\b\\c", NULL}; + const char *const a4[] = {"a\\\"b", "\"", "\\\\\"", NULL}; + const char *const a5[] = {"C:\\Users\\me\\my repo", "{\"name\":\"a b\",\"x\":\"y\\\\z\"}", + NULL}; + const char *const a6[] = {"", "plain", NULL}; + ASSERT(cmdline_roundtrips(a1)); + ASSERT(cmdline_roundtrips(a2)); + ASSERT(cmdline_roundtrips(a3)); + ASSERT(cmdline_roundtrips(a4)); + ASSERT(cmdline_roundtrips(a5)); + ASSERT(cmdline_roundtrips(a6)); + PASS(); +} + +/* Overflow is reported (false), never a silent truncation that would spawn a + * corrupted command line. */ +TEST(win_cmdline_overflow_rejected) { + const char *const argv[] = {"aaaaaaaaaa", "bbbbbbbbbb", NULL}; + char tiny[8]; + ASSERT_FALSE(cbm_build_win_cmdline(tiny, sizeof(tiny), argv)); + PASS(); +} + SUITE(subprocess) { RUN_TEST(subprocess_classify_clean); RUN_TEST(subprocess_classify_exit_nonzero); @@ -182,4 +307,7 @@ SUITE(subprocess) { RUN_TEST(subprocess_run_hang_is_hang); RUN_TEST(subprocess_run_spawn_failure); RUN_TEST(subprocess_run_null_bin_rejected); + RUN_TEST(win_cmdline_index_worker_json); + RUN_TEST(win_cmdline_roundtrip_battery); + RUN_TEST(win_cmdline_overflow_rejected); }