shell: fail the command when a > ${buf} redirect overflows the target Buffer - #34698
shell: fail the command when a > ${buf} redirect overflows the target Buffer#34698robobun wants to merge 6 commits into
> ${buf} redirect overflows the target Buffer#34698Conversation
Redirecting into a JS Buffer/TypedArray that is too small to hold the
output (`echo hello world > ${Buffer.alloc(4)}`) silently truncated
the output and exited 0, for both builtins and external commands. The
ENOSPC error was computed and then discarded.
Builtin path: `BuiltinIO::write_no_io_to` now returns ENOSPC on a
short write (after writing what fits), `Builtin::write_no_io` latches
the first such error on the `Builtin`, and `Builtin::done` turns an
otherwise-0 exit into 1 with a "<cmd>: No space left on device"
stderr message (when stderr is captured).
Subprocess path: `BufferedOutput::ArrayBuffer` tracks overflow in
`append`, and `PipeReader::try_signal_done_to_cmd` surfaces it via
the existing SystemError path in `Cmd::buffered_output_close`, which
sets the exit code to the errno.
|
Status: tests in CI (build 77906): all shell tests pass on every lane. Remaining red is unrelated: |
|
Updated 11:13 AM PT - Jul 22nd, 2026
❌ @robobun, your commit 9bc535c has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34698That installs a local version of the PR into your bun-34698 --bun |
WalkthroughChangesThe shell now detects output truncation for fixed-size buffers, preserves partial writes, reports Buffer overflow errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 466-469: Extend the “redirect into a too-small Buffer fails the
command” tests to cover standalone Uint8Array redirects, using a view with a
nonzero byte offset. Add both overflow and exact-fit cases, asserting overflow
returns a nonzero exit code while exact-fit succeeds with the expected bytes,
and preserve the existing Buffer coverage.
- Around line 503-508: Strengthen the assertions in both subprocess tests at
test/js/bun/shell/bunshell.test.ts:503-508 and
test/js/bun/shell/bunshell.test.ts:510-517 to verify the specific ENOSPC-derived
exit result and expected stderr error, replacing the generic nonzero exit check.
Ensure the fallback case asserts the same ENOSPC failure while preserving its
existing stderr behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d9922be-7e74-469f-a41f-af083e14cba5
📒 Files selected for processing (3)
src/runtime/shell/Builtin.rssrc/runtime/shell/subproc.rstest/js/bun/shell/bunshell.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: the duplicate-finder flag for #34699. Both PRs fix the same
The original report explicitly calls out both the builtin and the external-command path ("Same for the external-command path"), which only this PR addresses. Happy to defer to whichever shape reviewers prefer for the builtin side; the subprocess fix here is independent either way. |
|
This supersedes #34699 (same audit item, opened in parallel); the latch-on- Two other cheap hardening items from the same audit that #34699 carried and aren't here, in case you want to fold them in (neither has a fail-before test on its own): --- a/packages/bun-usockets/src/loop.c
+++ b/packages/bun-usockets/src/loop.c
@@ -75,3 +75,3 @@
uint64_t us_internal_monotonic_ns(void) {
- struct timespec ts;
+ struct timespec ts = {0, 0};
clock_gettime(CLOCK_MONOTONIC, &ts);
--- a/src/jsc/bindings/vm/SigintWatcher.cpp
+++ b/src/jsc/bindings/vm/SigintWatcher.cpp
@@ -87,3 +87,3 @@
WTF::Thread* currentThread = WTF::Thread::currentMayBeNull();
- ASSERT(!currentThread || m_thread->uid() != currentThread->uid());
+ ASSERT(!currentThread || !m_thread || m_thread->uid() != currentThread->uid());
@@ -102,3 +102,5 @@
m_semaphore.signal();
- m_thread->waitForCompletion();
+ if (m_thread) {
+ m_thread->waitForCompletion();
+ }( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- leak.test.ts: the ArrayBuffer/Buffer memleak cases intentionally overflow the target; accept the now-nonzero exit and stderr since the test is about the Strong ref being released. - Builtin::done: when stderr is an fd, tee the diagnostic into the captured buffer so r.stderr carries it on the non-.quiet() path (the terminal write stays skipped to keep done() synchronous). Covered by a new non-.quiet() test. - subproc: use to_shell_system_error() (static fields for a from_code error) so the synthesized ENOSPC does not leak a WTFStringImpl. - Cmd::buffered_output_close_*: do not overwrite an already-nonzero exit code with a pipe error, and deref() the SystemError. - Builtin::write_no_io: update the doc comment to match the new short-write contract.
There was a problem hiding this comment.
All three rounds of my earlier findings (fd-stderr tee, to_shell_system_error() + e.deref() at the sink, exit-code clobber guard, doc comment, and the leak.test.ts update) are addressed in 42b4510. No new issues found on this pass. Deferring to a maintainer for the user-facing behavior change and the unsafe { fd.captured_mut() } write in Builtin::done.
What was reviewed:
write_no_io_to/BufferedOutput::appendshort-write semantics and thewrite_errlatch →done()fold; verified builtin's own nonzero exit is preserved via(code, _) => code.- Subprocess ENOSPC path:
to_shell_system_error()is all-static for afrom_codeerror, and the newe.deref()inbuffered_output_close_{stdout,stderr}also fixes the pre-existingPipeReaderState::Err/captured_writer.errleak. captured_mut()tee indone(): single-threaded, no other borrow of the env's stderr bytelist live at that point; matches the existing pattern inbuffered_output_close_stdout.- Test coverage: builtin/subprocess, exact-fit boundary, offset
Uint8Arrayview with out-of-window bytes untouched,|| rhsfallback, non-.quiet()capture;leak.test.tsoverflow sites now ignore exit/stderr.
Extended reasoning...
Overview
Changes > ${buf} redirect semantics so overflowing a fixed-size Buffer/TypedArray target fails the command (exit 1 for builtins with a "No space left on device" diagnostic; ENOSPC-derived exit for subprocesses) instead of silently truncating and exiting 0. Touches Builtin.rs (short-write ENOSPC + latch + done() fold with fd-stderr tee), subproc.rs (BufferedOutput::ArrayBuffer overflow flag surfaced via try_signal_done_to_cmd), Cmd.rs (guard against clobbering an already-nonzero subprocess exit; e.deref() at the sink), plus tests in bunshell.test.ts and updates to leak.test.ts.
Security risks
None. No untrusted-input parsing, no auth/crypto, no path handling. The change tightens error reporting on a JS-only redirect target.
Level of scrutiny
Medium-high. This is a user-facing behavior change to Bun.$ (previously exit 0, now nonzero) and touches the shell interpreter's unsafe Rust: captured_mut() ((&self) -> &mut Vec<u8>), SystemError refcount handling, and the Cmd/PipeReader completion ordering. The mechanics look correct across the paths I traced, but the API-contract decision (overflow is a hard failure, mirroring > /dev/full) and the choice to skip the terminal write in the fd-stderr case (documented in the code comment) are design calls a maintainer should sign off on.
Other factors
I've reviewed this across three prior passes; the author addressed each finding (fd-stderr diagnostic reaching r.stderr in the non-.quiet() case, the WTFStringImpl leak via to_shell_system_error() + sink deref(), the subprocess exit-code clobber, the stale doc comment, and the leak.test.ts breakage). The added e.deref() in buffered_output_close_* also fixes a pre-existing leak on the PipeReaderState::Err path — I checked fill_system_error_common to confirm static/empty-string deref() is a no-op so this is safe for the new to_shell_system_error() path too. Test coverage is solid (both entry points, exact-fit boundary, offset view, || fallback, non-quiet capture). #34699 was closed in favor of this PR. CI build #75752 was in flight at review time.
…code
Stash the ArrayBuffer-overflow flag on SubprocExec and fold it into the
exit code at CmdState::Done, where the process's own code is known. This
makes the subprocess path deterministic and symmetric with Builtin::done:
a subprocess that both overflows the redirect target and exits nonzero
keeps its own exit code, regardless of whether pipe-EOF or process-exit
arrives first.
buffered_output_close_{stdout,stderr} keep the e.deref() for the
pre-existing PipeReaderState::Err path.
There was a problem hiding this comment.
LGTM — the latch-on-SubprocExec rework in 9bc535c resolves the last ordering concern; all prior review feedback is addressed and the bug hunter found nothing new on this revision.
What was reviewed:
redirect_overflowfold atCmdState::Doneis order-independent (fires only after both process exit and all pipes closed viahas_finished()); symmetric withBuiltin::done's(code, _) => codeguard.- Overflow path no longer synthesizes a
SystemError, so the earlierWTFStringImplleak is gone; the retainede.deref()/unwrap_or(0) == 0inbuffered_output_close_*are strict improvements to the pre-existing pipe-read-error path. - New "own nonzero exit wins over overflow" test pins the precedence; non-
.quiet()diagnostic tee, exact-fit, offset-view, and|| rhscases all covered.
Extended reasoning...
Overview
Fixes silent truncation when redirecting shell output into a too-small Buffer/TypedArray (> ${buf}): both the builtin path (Builtin.rs) and subprocess path (subproc.rs / Cmd.rs) now fail the command instead of exiting 0 with clipped data. Builtins latch the first write_no_io ENOSPC on Builtin::write_err and fold it into a nonzero exit + stderr diagnostic in Builtin::done; subprocesses set SubprocExec::redirect_overflow from BufferedOutput::append and fold it into ENOSPC at CmdState::Done only when the process itself exited 0. leak.test.ts updated for the new nonzero exit; ~110 lines of new tests in bunshell.test.ts.
Security risks
None. No untrusted-input parsing, no auth/crypto/permissions. The change tightens behavior (fail loud instead of silently truncate).
Level of scrutiny
Medium-high: shell interpreter internals with raw-pointer / Arc interior-mutability patterns. Mitigated by: this PR has been through four rounds of my own review (stale doc comment, non-.quiet() diagnostic gap, to_system_error() leak + exit-code clobber ordering A, exit-code clobber ordering B), each addressed with a targeted fix plus a covering test. The final latch-at-Done shape is the cleanest of the iterations — no SystemError synthesis, deterministic regardless of pipe-EOF vs. process-exit ordering, and symmetric with the builtin path's precedence rule.
Other factors
- Test coverage hits the variant matrix REVIEW.md asks for: builtin + subprocess,
.quiet()+ default fd stderr, exact-fit + overflow,Buffer+ offsetUint8Arrayview (with out-of-window bytes asserted untouched),|| rhsshort-circuit, and the new subprocess-nonzero-exit-wins-over-overflow test that pins the ordering fix. - Known design asymmetry (builtin → exit 1 +
"<cmd>: No space left on device"; subprocess → exit ENOSPC, no stderr) was surfaced in the PR description and earlier review and is consistent with the respective sibling error paths (on_io_writer_chunkvs.buffered_output_closeerrno). - Terminal-write skip in the non-
.quiet()builtin case is documented in the code comment and covered by a test assertingr.stderrstill carries the diagnostic. - The
e.deref()addition fixes a pre-existingSystemErrorstring leak on the real-read-error path — a rider improvement, not a regression risk. - CodeRabbit's two comments (TypedArray-view coverage, ENOSPC-specific assertion) were addressed; the outstanding CodeRabbit note on subprocess ENOSPC assertion strength is now satisfied by the
os.constants.errno.ENOSPCexact-match assertion.
1 participant
What does this PR do?
Redirecting shell output into a JS
Buffer/TypedArraythat is too small to hold it silently truncates the data and exits 0. The script has no way to detect that its capture was cut short.Repro
Same for the external-command path (
/bin/echo hello world > ${buf}-> exit 0,"hell").For comparison, an fd target already fails the command:
Cause
Builtin path:
BuiltinIO::write_no_io_toreturnedOk(short_len)for a partial write into anArrayBuftarget (only returningErr(ENOSPC)once the buffer was already full on entry), and every builtin call site discards the result (let _ = Builtin::write_no_io(...)) beforedone(interp, cmd, 0).Subprocess path:
BufferedOutput::appendsilently clipped to the remaining space (with a// TODO: We should probably throw error here?comment) and nothing propagated to the Cmd's exit code.Fix
Builtin path:
write_no_io_tonow returnsErr(ENOSPC)when not all bytes fit (after writing what does fit).Builtin::write_no_iolatches the first such error on theBuiltin, andBuiltin::doneturns an otherwise-0 exit into 1 with a"<cmd>: No space left on device"stderr message (when stderr is captured). This covers every builtin that writes viawrite_no_io(echo,seq,pwd,which,basename,dirname,ls,mkdir,touch,cat,cp,rm,export) without touching each call site.Subprocess path:
BufferedOutput::ArrayBuffertracks overflow inappend;PipeReader::try_signal_done_to_cmdsurfaces it via the existingSystemErrorpath inCmd::buffered_output_close, which sets the exit code to the errno (ENOSPC).Verification
The separate
> /dev/fullexit-code oddity (65508 instead of a small positive value) is the fd IOWriter path and is handled by #32278; not touched here.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts test/js/bun/shell/leak.test.ts