Skip to content

Commit 509c5b2

Browse files
committed
orchestrator: fix off-by-one in component output reader
A component line of >= LINE_LEN-1 bytes with no newline filled the read buffer; the next read() then had a zero-length count and returned 0, indistinguishable from EOF, and the EOF-tail path appended a byte one past buf (a 1-byte stack overflow — confirmed by AddressSanitizer: "stack-buffer-overflow WRITE of size 1 in run_component"). Reachable via long kernel-log / sysfs lines that components echo. Rewrite the read loop length-based: read into the free tail, split lines with memchr, and route every line through one handle_component_line() helper — for complete lines, the unterminated EOF tail, and an over-long line that fills the buffer. Over-long lines are now also streamed to verbose / captured in the component log instead of lost.
1 parent cb528bc commit 509c5b2

1 file changed

Lines changed: 92 additions & 82 deletions

File tree

src/orchestrator.c

Lines changed: 92 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,6 +1188,62 @@ static void apply_skip_filter(void) {
11881188
}
11891189
#endif /* !KASLD_TESTING */
11901190

1191+
/* Process one component output line (content only, no trailing newline): stream
1192+
* it to verbose stdout, capture it into the per-component log, and feed it to
1193+
* the address/scalar parser. Returns the number of records tagged (0 or 1).
1194+
* `content` need not be NUL-terminated; exactly `len` bytes are used. Input
1195+
* longer than the line buffer is truncated (the parser rejects malformed
1196+
* lines), so the single fixed copy never overflows. Called for each complete
1197+
* line, the unterminated EOF tail, and an over-long line that fills the read
1198+
* buffer without a newline — one place, no synthetic delimiters. */
1199+
static int handle_component_line(struct component_log *clog,
1200+
const char *comp_method, const char *origin,
1201+
const char *content, size_t len) {
1202+
char line[LINE_LEN];
1203+
if (len >= sizeof(line))
1204+
len = sizeof(line) - 1;
1205+
memcpy(line, content, len);
1206+
line[len] = '\0';
1207+
1208+
if (verbose && !json_output)
1209+
printf("%s\n", line);
1210+
1211+
/* Capture line for verbose / JSON-with-output. Allocated on first use and
1212+
* grown geometrically — no fixed cap, so noisy components do not silently
1213+
* lose their tail. Non-verbose runs never enter this branch and never
1214+
* allocate. Allocation failures degrade gracefully: the line is dropped (and
1215+
* counts as truncated), but capture continues for subsequent lines. */
1216+
if (clog && verbose) {
1217+
if (clog->num_lines >= clog->lines_cap) {
1218+
int new_cap =
1219+
clog->lines_cap ? clog->lines_cap * 2 : COMPONENT_LINES_INITIAL_CAP;
1220+
char **bigger = realloc(clog->lines, (size_t)new_cap * sizeof(char *));
1221+
if (bigger) {
1222+
clog->lines = bigger;
1223+
clog->lines_cap = new_cap;
1224+
} else {
1225+
orchestrator_saturation |= ORCH_SAT_COMPONENT_LINES_DROPPED;
1226+
}
1227+
}
1228+
if (clog->num_lines < clog->lines_cap) {
1229+
char *copy = malloc(MAX_LINE_LEN);
1230+
if (copy) {
1231+
snprintf(copy, MAX_LINE_LEN, "%s", line);
1232+
clog->lines[clog->num_lines++] = copy;
1233+
} else {
1234+
orchestrator_saturation |= ORCH_SAT_COMPONENT_LINES_DROPPED;
1235+
}
1236+
}
1237+
}
1238+
1239+
/* Origin (provenance) is the component name — captured at the orchestrator
1240+
* since it owns the subprocess identity. `S` lines are scalar system facts;
1241+
* everything else is an address record. */
1242+
if (line[0] == 'S')
1243+
return capture_scalar(line, origin);
1244+
return capture_result(line, comp_method, origin);
1245+
}
1246+
11911247
static int run_component(const struct component *c) {
11921248
/* Extract explain string before execution (if --explain active or JSON) */
11931249
char *explain_str = NULL;
@@ -1319,95 +1375,49 @@ static int run_component(const struct component *c) {
13191375
break;
13201376
}
13211377

1322-
/* Read available data */
1323-
ssize_t n = read(pipefd[0], buf + buf_pos, sizeof(buf) - buf_pos - 1);
1324-
int eof_flush = 0;
1378+
/* Read available data into the free tail of the buffer. */
1379+
ssize_t n = read(pipefd[0], buf + buf_pos, sizeof(buf) - buf_pos);
13251380
if (n <= 0) {
1326-
/* EOF or error. Promote any unterminated tail to a synthetic final
1327-
* line so it reaches the per-line handler below: capture_result /
1328-
* capture_scalar parse-reject malformed input cleanly (return 0),
1329-
* which surfaces a segfault-mid-line as a recordable parse failure
1330-
* rather than a silent drop. The line is also streamed to verbose
1331-
* stdout and captured into the per-component log, matching the
1332-
* normal path. The synthetic '\n' fits because the read leaves at
1333-
* least one byte of headroom (`sizeof(buf) - buf_pos - 1` was the
1334-
* read length, so buf_pos < sizeof(buf) - 1 holds). */
1335-
if (buf_pos == 0 || buf[buf_pos - 1] == '\n')
1336-
break; /* nothing to flush, or already cleanly terminated */
1337-
buf[buf_pos++] = '\n';
1338-
buf[buf_pos] = '\0';
1339-
eof_flush = 1; /* break out after processing this final line */
1340-
} else {
1341-
buf_pos += (size_t)n;
1342-
buf[buf_pos] = '\0';
1381+
/* EOF or error. Flush any unterminated tail as a final line so it
1382+
* reaches the parser — which rejects malformed input cleanly (return 0),
1383+
* surfacing a segfault-mid-line as a recordable parse failure rather than
1384+
* a silent drop — plus the verbose / per-component-log path. A cleanly
1385+
* terminated stream ends on a newline that the loop below already
1386+
* consumed, leaving buf_pos == 0 and nothing to flush. */
1387+
if (buf_pos > 0)
1388+
tagged_this_run +=
1389+
handle_component_line(clog, comp_method, c->name, buf, buf_pos);
1390+
break;
13431391
}
1392+
buf_pos += (size_t)n;
13441393

1345-
/* Process complete lines */
1346-
char *start = buf;
1394+
/* Hand off each complete (newline-terminated) line; the newline itself is
1395+
* not part of the content. memchr is length-bounded, so buf needs no NUL
1396+
* terminator and an embedded NUL cannot truncate a line. */
1397+
size_t start = 0;
13471398
char *nl;
1348-
while ((nl = strchr(start, '\n')) != NULL) {
1349-
*nl = '\0';
1350-
if (verbose && !json_output)
1351-
printf("%s\n", start);
1352-
1353-
/* Capture line for verbose / JSON-with-output. Allocated on first use
1354-
* and grown geometrically — no fixed cap, so noisy components do not
1355-
* silently lose their tail. Non-verbose runs never enter this branch
1356-
* and never allocate. Allocation failures degrade gracefully: the line
1357-
* is dropped (and counts as truncated), but capture continues for
1358-
* subsequent lines. */
1359-
if (clog && verbose) {
1360-
if (clog->num_lines >= clog->lines_cap) {
1361-
int new_cap = clog->lines_cap ? clog->lines_cap * 2
1362-
: COMPONENT_LINES_INITIAL_CAP;
1363-
char **bigger =
1364-
realloc(clog->lines, (size_t)new_cap * sizeof(char *));
1365-
if (bigger) {
1366-
clog->lines = bigger;
1367-
clog->lines_cap = new_cap;
1368-
} else {
1369-
orchestrator_saturation |= ORCH_SAT_COMPONENT_LINES_DROPPED;
1370-
}
1371-
}
1372-
if (clog->num_lines < clog->lines_cap) {
1373-
char *copy = malloc(MAX_LINE_LEN);
1374-
if (copy) {
1375-
snprintf(copy, MAX_LINE_LEN, "%s", start);
1376-
clog->lines[clog->num_lines++] = copy;
1377-
} else {
1378-
orchestrator_saturation |= ORCH_SAT_COMPONENT_LINES_DROPPED;
1379-
}
1380-
}
1381-
}
1382-
1383-
/* Re-add newline for capture (region newline stripped in capture_result)
1384-
*/
1385-
*nl = '\n';
1386-
char line[LINE_LEN];
1387-
size_t llen = (size_t)(nl - start + 1);
1388-
if (llen < sizeof(line)) {
1389-
memcpy(line, start, llen);
1390-
line[llen] = '\0';
1391-
/* Origin (provenance) is the component name — captured at the
1392-
* orchestrator since it owns the subprocess identity. `S` lines are
1393-
* scalar system facts; everything else is an address record. */
1394-
if (line[0] == 'S')
1395-
tagged_this_run += capture_scalar(line, c->name);
1396-
else
1397-
tagged_this_run += capture_result(line, comp_method, c->name);
1398-
}
1399-
1400-
start = nl + 1;
1399+
while ((nl = memchr(buf + start, '\n', buf_pos - start)) != NULL) {
1400+
size_t llen = (size_t)(nl - (buf + start));
1401+
tagged_this_run +=
1402+
handle_component_line(clog, comp_method, c->name, buf + start, llen);
1403+
start = (size_t)(nl - buf) + 1;
1404+
}
1405+
size_t left = buf_pos - start;
1406+
1407+
/* A line longer than the whole buffer has no newline to split on — the
1408+
* only way `left` can reach the buffer size. Flush the buffered prefix as a
1409+
* (truncated) line so the reader makes progress instead of stalling on a
1410+
* zero-length read, then keep reading the rest of the line. */
1411+
if (left == sizeof(buf)) {
1412+
tagged_this_run +=
1413+
handle_component_line(clog, comp_method, c->name, buf, left);
1414+
left = 0;
14011415
}
14021416

1403-
/* Shift remaining partial line to front of buffer */
1404-
size_t left = buf_pos - (size_t)(start - buf);
1405-
if (left > 0)
1406-
memmove(buf, start, left);
1417+
/* Shift any remaining partial line to the front of the buffer. */
1418+
if (left > 0 && start > 0)
1419+
memmove(buf, buf + start, left);
14071420
buf_pos = left;
1408-
1409-
if (eof_flush)
1410-
break; /* EOF tail was just flushed as a synthetic final line */
14111421
}
14121422

14131423
close(pipefd[0]);

0 commit comments

Comments
 (0)