Merge pull request #881 from Flipper1994/fix/win-index-worker-cmdline-quoting

fix(win): escape spawned command-line args so the index worker gets valid argv

Author: DeusData

src/foundation/subprocess.c

@@ -132,25 +132,107 @@ 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) {
     const char *bin = opts->bin;
     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;

src/foundation/subprocess.h

@@ -21,6 +21,7 @@
 #define CBM_SUBPROCESS_H
 
 #include <stdbool.h>
+#include <stddef.h> /* 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 */

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;
 }
 

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 <sqlite3/sqlite3.h>
 #include <yyjson/yyjson.h>
@@ -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);
 

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);
 }