Skip to content

Commit a57d8e3

Browse files
committed
fix(subprocess): NUL-terminate cbm_build_win_cmdline buf on overflow
Follow-up to #881. cbm_build_win_cmdline() returned false from inside its loop on buffer overflow without NUL-terminating buf, although its header contract states the buffer holds a string. No live bug — both call sites (cbm_run_win and the UI http_server index spawn) ignore buf when the function returns false — but a future caller trusting the documented contract could read an unterminated (possibly uninitialized) buffer. Set buf[0]='\0' on the overflow path so it is always a valid string, and correct the header comment to match. Reproduce-first: the new win_cmdline_overflow_leaves_empty_string test feeds an argv that overflows a small cap and asserts buf[0]=='\0'. It is RED on the pre-fix code (buf[0] holds the first quoted byte '"' = 0x22) and GREEN with the fix. Also tidies two review nits in the same files: - fix the stale cbm_cmdline_put comment (it returns pos UNCHANGED on overflow; the overflow is signalled via *ovf, not an advancing pos), - bounds-guard the reference CommandLineToArgvW parser in the round-trip test. Refs: #881 Signed-off-by: Flipper <jacobphilipp@ymail.com>
1 parent 582c124 commit a57d8e3

3 files changed

Lines changed: 33 additions & 7 deletions

File tree

src/foundation/subprocess.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,8 @@ static bool cbm_tail_log(const char *log_file, long *tail_pos, cbm_proc_log_cb c
135135
/* ── Windows command-line quoting (pure; unit-tested on every platform) ─────── */
136136

137137
/* Append char `c` to buf[cap], reserving the final byte for a NUL terminator.
138-
* Sets *ovf on overflow and stops writing; pos keeps advancing so the caller
139-
* still detects the overflow after the loop. */
138+
* On overflow: sets *ovf, stops writing, and returns pos UNCHANGED — callers detect
139+
* the overflow via the *ovf flag (not via the return value). */
140140
static size_t cbm_cmdline_put(char *buf, size_t cap, size_t pos, char c, bool *ovf) {
141141
if (pos + 1 >= cap) {
142142
*ovf = true;
@@ -213,6 +213,7 @@ bool cbm_build_win_cmdline(char *buf, size_t cap, const char *const *argv) {
213213
for (int i = 0; argv[i]; i++) {
214214
pos = cbm_cmdline_append_arg(buf, cap, pos, argv[i], i == 0, &ovf);
215215
if (ovf) {
216+
buf[0] = '\0'; /* overflow: leave buf a valid (empty) string, never unterminated */
216217
return false;
217218
}
218219
}

src/foundation/subprocess.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ const char *cbm_proc_outcome_str(cbm_proc_outcome_t o);
7777
/* Build a Windows CreateProcess command line from a NULL-terminated argv, applying
7878
* the Microsoft C runtime quoting rules (quote-wrap + escape embedded quotes and
7979
* their preceding backslashes) so the spawned child re-parses byte-identical argv.
80-
* Returns true on success, false on overflow (buf then holds a truncated string).
80+
* Returns true on success, false on overflow (on overflow buf is set to an empty
81+
* string, never left unterminated).
8182
*
8283
* CreateProcess re-parses a SINGLE command string into argv, so a naive `"%s"` wrap
8384
* silently corrupts any element containing a double-quote — e.g. the index worker's

tests/test_subprocess.c

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,15 @@ static int parse_win_cmdline(const char *cmd, char out[][256], int max_args) {
195195
char *o = out[argc];
196196
size_t oi = 0;
197197
bool in_quotes = false;
198+
/* Guard every write: each out[] row is 256 bytes; test args stay well under
199+
* that, but cap defensively so a future longer arg fails a length assertion
200+
* rather than smashing the stack. */
201+
#define PUTO(ch) \
202+
do { \
203+
if (oi < 255) { \
204+
o[oi++] = (ch); \
205+
} \
206+
} while (0)
198207
for (;;) {
199208
size_t nbs = 0;
200209
while (*p == '\\') {
@@ -203,24 +212,26 @@ static int parse_win_cmdline(const char *cmd, char out[][256], int max_args) {
203212
}
204213
if (*p == '"') {
205214
for (size_t k = 0; k < nbs / 2; k++) {
206-
o[oi++] = '\\';
215+
PUTO('\\');
207216
}
208217
if (nbs % 2) {
209-
o[oi++] = '"'; /* odd run → the quote is an escaped literal */
218+
PUTO('"'); /* odd run → the quote is an escaped literal */
210219
} else {
211220
in_quotes = !in_quotes; /* even run → the quote is a delimiter */
212221
}
213222
p++;
214223
} else {
215224
for (size_t k = 0; k < nbs; k++) {
216-
o[oi++] = '\\';
225+
PUTO('\\');
217226
}
218227
if (*p == '\0' || (!in_quotes && (*p == ' ' || *p == '\t'))) {
219228
break;
220229
}
221-
o[oi++] = *p++;
230+
PUTO(*p);
231+
p++;
222232
}
223233
}
234+
#undef PUTO
224235
o[oi] = '\0';
225236
argc++;
226237
}
@@ -293,6 +304,18 @@ TEST(win_cmdline_overflow_rejected) {
293304
PASS();
294305
}
295306

307+
/* Reproduce-first guard for the overflow CONTRACT (subprocess.h): on overflow buf
308+
* must be left a valid (empty) string. RED on the pre-fix code — the overflow path
309+
* returned without terminating buf, so buf[0] held the first quoted byte ('"') —
310+
* and GREEN once the overflow path sets buf[0] = '\0'. */
311+
TEST(win_cmdline_overflow_leaves_empty_string) {
312+
char buf[8];
313+
const char *const argv[] = {"averylongprogramname", "x", NULL};
314+
ASSERT_FALSE(cbm_build_win_cmdline(buf, sizeof(buf), argv)); /* overflows cap */
315+
ASSERT_EQ(buf[0], '\0'); /* pre-fix: buf[0] == '"' (a partial byte), not NUL */
316+
PASS();
317+
}
318+
296319
SUITE(subprocess) {
297320
RUN_TEST(subprocess_classify_clean);
298321
RUN_TEST(subprocess_classify_exit_nonzero);
@@ -310,4 +333,5 @@ SUITE(subprocess) {
310333
RUN_TEST(win_cmdline_index_worker_json);
311334
RUN_TEST(win_cmdline_roundtrip_battery);
312335
RUN_TEST(win_cmdline_overflow_rejected);
336+
RUN_TEST(win_cmdline_overflow_leaves_empty_string);
313337
}

0 commit comments

Comments
 (0)