child_process: latch exec/execFile maxBuffer overflow so truncated output never exceeds the cap

[robobun]

What

exec/execFile with a fast writer and a maxBuffer cap could hand the callback a stdout/stderr up to ~3x the cap (still with ERR_CHILD_PROCESS_STDIO_MAXBUFFER). Node and Bun 1.4.0 both truncate to exactly maxBuffer.

Repro

import { execFile } from "node:child_process";
execFile("head", ["-c", "4194304", "/dev/zero"], { maxBuffer: 65536, encoding: "buffer" }, (err, stdout) => {
  console.log(err?.code, stdout.length); // ERR_CHILD_PROCESS_STDIO_MAXBUFFER 131072  (expected 65536)
});

Cause

027716a ("stdin: apply highwater backpressure to the pipe FileReader source") made the child's stdout FileReader deliver ~64 KiB chunks with the next one already queued, so a second data event can fire after execFile's kill()/stdout.destroy(). The onData handler has no latch; on that second event totalLen - length is already past maxBuffer, so

const truncatedLen = maxBuffer - (totalLen - length); // negative
$arrayPush(_buffer, chunk.slice(0, truncatedLen));    // slice(0, -k) keeps chunk.length - k bytes

appends past the cap and calls kill() again. Node has the same arithmetic but its destroy() stops further data events; this was a latent bug in the JS handler exposed by the chunking change.

Fix

Add a per-stream maxBufferTripped latch so the handler drops every chunk after the first overflow (error and kill() fire once), and clamp truncatedLen at zero.

Verification

New test in test/js/node/child_process/child-process-exec.test.ts spawns head -c 4194304 /dev/zero ten times concurrently for each of encoding: "buffer" and "utf8" and asserts every callback receives { code: "ERR_CHILD_PROCESS_STDIO_MAXBUFFER", len: 65536 }.

Without the fix (src/ stashed): 10/10 runs over-capture (126976..131072 bytes) in both encodings.
With the fix: 10/10 exact, all 13 tests in the file pass.

---

no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/child_process/child-process-exec.test.ts

[robobun]

Status: rebased onto main@e532ad91f (323ce62), gate proof re-verified locally. CI red on unrelated flaky lanes only.

Reproduced with execFile("head", ["-c", "4194304", "/dev/zero"], { maxBuffer: 65536 }) on a debug build of main: 10/10 over-capture (126976..131072 bytes) without the src/ change, 10/10 exact 65536 with it. Full child-process-exec.test.ts suite (13 tests) passes.

CI (build 84394): no [new] failures and nothing in exec/execFile maxBuffer. All 13 red groups are [flaky] parallel-batch failures that passed when run alone (watch-many-dirs, numeric-header, fastutf8stream-reopen, socket-retention, 20875, 08965, request-smuggling, fetch-leak, spawn-streaming-stdout, proxy-stress-protocol, process-stdio-stack-overflow, 09279 spawn-timeout, resolve-long-cwd). Ready for a maintainer to merge past the unrelated noise.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ecefee67-1fb6-4e4b-98b7-cc6335f92177

📥 Commits

Reviewing files that changed from the base of the PR and between e532ad9 and 323ce62.

📒 Files selected for processing (2)

• src/js/node/child_process.ts
• test/js/node/child_process/child-process-exec.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — small, well-scoped regression fix with a gate-proven test.

What was reviewed:

• Latch is per-stream (closure-local to addOnDataListener), so stdout and stderr trip independently; ex/kill() fire once per stream as before.
• With the latch in place totalLen - length (the pre-chunk total) is always ≤ maxBuffer on the tripping chunk, so truncatedLen can't go negative — the MathMax(0, …) clamp is defensive but harmless.
• MathMax captured at module load matches the file's primordial pattern; test uses head -c … /dev/zero (POSIX-only, correctly skipIf(isWindows)) with bunEnv and asserts an exact 10-element result array for both encodings.

Extended reasoning...

Overview

Two-file change: src/js/node/child_process.ts adds a per-stream maxBufferTripped boolean latch inside execFile's addOnDataListener closure so that once a stream overflows maxBuffer, subsequent data chunks are dropped instead of re-running the (now negative) truncation arithmetic and calling kill() again. truncatedLen is additionally clamped at zero via a module-load-captured MathMax. The test file gains a describe.concurrent.each over "buffer"/"utf8" that spawns 10 concurrent head -c 4194304 /dev/zero children with a 64 KiB cap and asserts every callback receives exactly { code: "ERR_CHILD_PROCESS_STDIO_MAXBUFFER", len: 65536 }.

Security risks

None. No untrusted-input parsing, no auth/crypto/permissions surface. The change only tightens an existing output-size cap.

Level of scrutiny

Low-to-moderate. This is a ~10-line JS-side fix in the Node-compat layer with a clear mechanical explanation (negative slice end index keeps a tail) and a demonstrated fail→pass gate. The latch lives in a closure so it's naturally per-stream and per-execFile call; both the encoding and buffer branches are updated symmetrically. The MathMax(0, …) is redundant given the latch (pre-chunk total is ≤ maxBuffer on the first overflow) but is cheap defense.

Other factors

• On layering: the invariant "callback output ≤ maxBuffer" is owned by execFile, so latching here (rather than changing stream destroy() semantics) is the right layer for this specific contract; the code comment documents why late chunks can arrive.
• Test follows harness conventions (bunEnv, test.skipIf(isWindows), Promise.withResolvers, concurrent, strong toEqual on the full result array). Windows is skipped because head//dev/zero aren't available; the fix itself is platform-agnostic JS and the existing slower-writer maxBuffer tests in the same file still run on Windows.
• PR description shows 10/10 over-capture without the fix and 10/10 exact with it; no prior reviewer comments to address.

[robobun]

^{Updated 6:20 PM PT - Jul 28th, 2026}

✅ @robobun, your commit 323ce62ed76f827eaa5ddbb1a275aaa3cb9b54d0 passed in Build #84394! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36169

That installs a local version of the PR into your bun-36169 executable, so you can run:

bun-36169 --bun

[claude]

LGTM — small, well-scoped latch on the execFile maxBuffer overflow path with a gate-proven regression test.

What was reviewed:

• The maxBufferTripped latch is per-stream (closure-local in addOnDataListener), so stdout and stderr trip independently; ex/kill() fire once per stream instead of on every post-destroy chunk.
• MathMax(0, ...) clamp is redundant given the latch (first-trip totalLen - length is always ≤ maxBuffer) but harmless defense-in-depth; MathMax primordial capture matches the file's existing pattern.
• Test uses head -c + /dev/zero (skipped on Windows), NUL bytes so byteLength == code-unit length for the utf8 case, and the callback always resolves so no hang path. PR description shows 10/10 over-capture without the fix, 10/10 exact with it.

Extended reasoning...

Overview

Fixes a regression in execFile/exec where a fast writer could deliver a second data chunk after kill()/destroy(), causing the negative-index slice(0, truncatedLen) to append past maxBuffer. The fix adds a boolean latch so the handler drops every chunk after the first overflow, plus a Math.max(0, ...) clamp on the truncation length. Touches one function in src/js/node/child_process.ts (~8 lines net) and adds one regression test block to the existing child-process-exec.test.ts.

Security risks

None. This is output-truncation bookkeeping in the Node compat layer; no auth, crypto, or untrusted-input parsing is involved. If anything it tightens a resource bound (output size) that was previously being exceeded.

Level of scrutiny

Low-to-medium. The mechanism is a trivial early-return guard on a closure-local boolean, applied identically to both the string-encoding and buffer branches. The root cause is clearly explained (027716a changed chunking so a queued chunk can arrive post-destroy; Node's identical arithmetic is protected by destroy() stopping further data events). The MathMax clamp is belt-and-suspenders — with the latch in place, totalLen - length on the first trip is the pre-chunk total, which was ≤ maxBuffer, so the subtraction is already non-negative.

Other factors

• The latch lives in addOnDataListener's closure, so stdout and stderr each get their own — no cross-stream interference.
• MathMax = Math.max captured at module load matches the file's existing primordial-capture convention (NumberIsInteger, etc.).
• Test is added to the existing file (not a new regression file), skips Windows appropriately for head(1)//dev/zero, uses bunEnv, and asserts an exact {code, len} array via toEqual across 10 concurrent runs × 2 encodings. For utf8, the writer emits NUL bytes so Buffer.byteLength and string .length agree, making the exact-65536 assertion sound.
• The comment-cop bot's feedback (paragraph-long comment) was addressed in 323ce62 and the thread is resolved.
• Gate proof in the PR: 10/10 over-capture on main without the src change, 10/10 exact with it; full file (13 tests) passes.

[robobun]

Status note from replaying this PR's tests against current main (04148c8): the maxBuffer cap with fast writer cases now pass without this fix, so the fail-before proof here no longer holds at maxBuffer = 64 KiB (chunk delivery changed after #36035). The bug itself is still present on main: with the same head -c writer and maxBuffer = 1 MiB, 27 of 320 runs returned stdout longer than maxBuffer (for example 1191936 bytes), and child_process.ts still computes truncatedLen without the latch. The test probably needs a larger maxBuffer (or to assert across a few sizes) to keep failing without the fix.