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>
This commit is contained in:
Flipper committed 2026-07-05 17:42:17 +02:00
1 parent 582c124a23
commit a57d8e3e9b
3 files changed
+33 -7

No files matched your search

+3 -2
View File
@@ -135,8 +135,8 @@ static bool cbm_tail_log(const char *log_file, long *tail_pos, cbm_proc_log_cb c
/* ── 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. */
* On overflow: sets *ovf, stops writing, and returns pos UNCHANGED — callers detect
* the overflow via the *ovf flag (not via the return value). */
static size_t cbm_cmdline_put(char *buf, size_t cap, size_t pos, char c, bool *ovf) {
if (pos + 1 >= cap) {
*ovf = true;
@@ -213,6 +213,7 @@ bool cbm_build_win_cmdline(char *buf, size_t cap, const char *const *argv) {
for (int i = 0; argv[i]; i++) {
pos = cbm_cmdline_append_arg(buf, cap, pos, argv[i], i == 0, &ovf);
if (ovf) {
buf[0] = '\0'; /* overflow: leave buf a valid (empty) string, never unterminated */
return false;
}
}
+2 -1
View File
@@ -77,7 +77,8 @@ 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).
* Returns true on success, false on overflow (on overflow buf is set to an empty
* string, never left unterminated).
*
* 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
+28 -4
View File
@@ -195,6 +195,15 @@ static int parse_win_cmdline(const char *cmd, char out[][256], int max_args) {
char *o = out[argc];
size_t oi = 0;
bool in_quotes = false;
/* Guard every write: each out[] row is 256 bytes; test args stay well under
* that, but cap defensively so a future longer arg fails a length assertion
* rather than smashing the stack. */
#define PUTO(ch) \
do { \
if (oi < 255) { \
o[oi++] = (ch); \
} \
} while (0)
for (;;) {
size_t nbs = 0;
while (*p == '\\') {
@@ -203,24 +212,26 @@ static int parse_win_cmdline(const char *cmd, char out[][256], int max_args) {
}
if (*p == '"') {
for (size_t k = 0; k < nbs / 2; k++) {
o[oi++] = '\\';
PUTO('\\');
}
if (nbs % 2) {
o[oi++] = '"'; /* odd run → the quote is an escaped literal */
PUTO('"'); /* 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++] = '\\';
PUTO('\\');
}
if (*p == '\0' || (!in_quotes && (*p == ' ' || *p == '\t'))) {
break;
}
o[oi++] = *p++;
PUTO(*p);
p++;
}
}
#undef PUTO
o[oi] = '\0';
argc++;
}
@@ -293,6 +304,18 @@ TEST(win_cmdline_overflow_rejected) {
PASS();
}
/* Reproduce-first guard for the overflow CONTRACT (subprocess.h): on overflow buf
* must be left a valid (empty) string. RED on the pre-fix code — the overflow path
* returned without terminating buf, so buf[0] held the first quoted byte ('"') —
* and GREEN once the overflow path sets buf[0] = '\0'. */
TEST(win_cmdline_overflow_leaves_empty_string) {
char buf[8];
const char *const argv[] = {"averylongprogramname", "x", NULL};
ASSERT_FALSE(cbm_build_win_cmdline(buf, sizeof(buf), argv)); /* overflows cap */
ASSERT_EQ(buf[0], '\0'); /* pre-fix: buf[0] == '"' (a partial byte), not NUL */
PASS();
}
SUITE(subprocess) {
RUN_TEST(subprocess_classify_clean);
RUN_TEST(subprocess_classify_exit_nonzero);
@@ -310,4 +333,5 @@ SUITE(subprocess) {
RUN_TEST(win_cmdline_index_worker_json);
RUN_TEST(win_cmdline_roundtrip_battery);
RUN_TEST(win_cmdline_overflow_rejected);
RUN_TEST(win_cmdline_overflow_leaves_empty_string);
}