shell: throw instead of panic on ReadableStream redirect#33996
Conversation
…palive on sync throw
Redirecting a ReadableStream to a subprocess in Bun Shell
(`$`cmd < ${stream}``) hit an explicit `panic!("TODO SHELL READABLE
STREAM")` in the subproc redirect dispatch, aborting the whole process.
Drop the placeholder branch so the value falls through to the existing
"Unknown JS value used in shell" TypeError, matching the builtin path.
Separately, `run_from_js` returned the thrown exception without
releasing the event-loop keepalive or undoing the pending-activity
increment, so even when the error was caught the process never exited.
Mirror the cleanup from `finish()` on that path.
|
Updated 2:33 AM PT - Jul 12th, 2026
❌ @robobun, your commit 8983519 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33996That installs a local version of the PR into your bun-33996 --bun |
WalkthroughShell failures now route pending JavaScript exceptions through the shell promise rejection callback, while early setup failures disable keep-alive state. ReadableStream redirects use the existing unknown-value error path and are covered across multiple redirect forms. ChangesShell runtime behavior
🚥 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 `@src/runtime/shell/interpreter.rs`:
- Around line 1358-1364: Remove the unconditional
Self::decr_pending_activity_flag call from the synchronous exception branch in
the interpreter flow, or guard it so it runs only when finish() will not later
execute; preserve keep_alive disabling and the thrown-error return. Ensure
async_cmd_done() → finish() remains the sole decrement path when async shell
work is still in flight.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 1556-1562: Update the parameterized test under “ReadableStream as
redirect target throws instead of panicking” to use concurrent execution, such
as test.concurrent.each, while preserving the existing cases and assertions.
🪄 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: 4c93c8ad-cafa-4a7f-985c-79a0ce717398
📒 Files selected for processing (3)
src/runtime/shell/interpreter.rssrc/runtime/shell/states/Cmd.rstest/js/bun/shell/bunshell.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/shell/states/Cmd.rs
The decrement let GC collect the interpreter while a spawned pipeline sibling's PipeReader still held a raw pointer into it, a UAF under ASAN. Release only the keepalive; the GC pin stays until async IO drains. Also: test.concurrent.each, add pipeline coverage with Bun.gc(true).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…line
Handling the thrown exception only in run_from_js missed every async
re-entry (Cmd::on_exit, PipeReader, IOWriter), so cmd0 && cmd1 < ${stream}
left a pending exception and hung. Yield::run now calls
Interpreter::reject_pending_exception on Failed, which takes the
exception, rejects the ShellPromise with it, and releases the keepalive.
has_pending_activity stays non-zero so GC cannot collect the interpreter
while a pipeline sibling's PipeReader still holds a raw pointer into it.
The JS-side reject callback was stored but never invoked from native;
repurpose it to forward its argument straight to the promise reject.
Drop the not.toContain('panic') assertion per repo convention; the
combined exitCode/signalCode/stdout object already fails on an abort.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/shell/interpreter.rs:1358-1364— d8911d6 traded the UAF for a leak:has_pending_activityis incremented at line 1350 and now never balanced on this path (the only decrement isDecrOnDropinsidefinish(), which is unreachable afterreturn Err), sohasPendingActivity()returns true forever and the JS wrapper — thus the wholeInterpreterbox — is permanently GC-pinned per failed invocation. The comment justifies the pin for a livePipeReader, but in 4 of the 5 new test cases (and the pre-existing builtinecho > ${stream}path) the throw happens before anything spawns, so there is nothing to drain and the pin is pure leak. The sharedYield::Failedteardown proposed in the earlier thread — kill/detach any liveExec::Subprocnodes, then decrement — resolves both this and the pipeline UAF.Extended reasoning...
What the bug is
Commit d8911d6 removed
decr_pending_activity_flagfrom the sync-throw branch ofrun_from_jsto avoid the pipeline UAF, leaving onlykeep_alive.disable(). But now the counter incremented at line 1350 is never balanced on this path: the sole decrement site in the file is theDecrOnDropguard insidefinish()(line 1276), andfinish()is only reached viaon_root_child_done(root Script completes) orasync_cmd_done— neither of which fires wheninit_subproc_redirectionsthrows before anything spawned and the trampoline returns onYield::failed(). Sohas_pending_activitystays at 1 forever,hasPendingActivity()(line 1617) returns true forever, and the JS wrapper is permanently pinned against GC.Why the earlier comment doesn't cover this
The existing thread on this line was written against 580a810, which did decrement here, and describes the UAF from dropping the counter to 0 while a pipeline sibling's
PipeReaderstill holds a raw backref. d8911d6 removed the decrement, so that analysis is stale against current HEAD. This finding is the inverse failure mode: by not decrementing, the non-pipeline cases now leak. The author's reply ("has_pending_activity stays non-zero so the interpreter is pinned until async IO drains") assumes there is async IO to drain — but in the common case there isn't, so nothing ever brings the counter back to 0.Step-by-step proof (non-pipeline case)
Take the first test case,
${process.execPath} -e 0 < ${stream}:run_from_jsline 1350:incr_pending_activity_flag→ counter = 1.Script::start(...).run(self)reachesCmd::transition_to_execfor the single command.init_subproc_redirectionssees theReadableStreamJS value, falls through toErr(global.throw("Unknown JS value used in shell: ...")), andtransition_to_execreturnsYield::failed()beforeShellSubprocess::spawn_asyncis called. No subprocess, noPipeReader, noExec::Subproc—me.execwas never assigned.Yield::runtreatsFailedlikeSuspended: it drains pipelines (none) and returns.- Back in
run_from_jsline 1358:global_this.has_exception()is true →keep_alive.disable()→return Err(JsError::Thrown). Counter still = 1. - In
src/js/builtins/shell.ts#run(),interpis a local (let interp = createShellInterpreter(...)) that is never stored onthis. Wheninterp.run()throws, the local goes out of scope. Nothing else roots the wrapper. Shell.classes.tssetshasPendingActivity: true, so the GC hook callshas_pending_activity()(line 1616-1618), which returns1 > 0= true. The wrapper is pinned.- Nothing will ever call
finish(): the root Script never reachedchild_done(its Cmd child is stuck inExecstate with no exec), and there are no async cmds. Counter stays 1 for the life of the process. finalize()→deinit_from_finalizernever runs. TheInterpreterbox (nodesVec<Node>,root_shellenv maps,root_io,jsobjs, plus the wrapper's cachedresolve/rejectvalues) leaks.
The same trace applies to
> ${stream},2> ${stream},< ${new Response("x").body}, and the pre-existing builtin pathecho > ${stream}(which threw fromBuiltin::init_redirectionsbefore this PR and already leaked — the PR widens the surface to the subprocess path).Why the pin is only sometimes needed
The comment at 1359-1361 is correct only for the pipeline case (
cmd0 | cmd1 < ${stream}), where cmd0 has already spawned and itsShellSubprocess/PipeReaderholds a rawinterpbackref — there, dropping to 0 lets GC free the interpreter under a live callback (the ASAN UAF the author reproduced). But in the 4 non-pipeline test cases the throw happens beforespawn_async, so there is no backref to protect and the pin is pure leak. An unconditional "leave it non-zero" is the wrong fix for both.Impact
Per-invocation heap leak on a user-reachable error path. A long-running process that catches these errors and continues (retry loop, REPL, dev server evaluating user snippets) accumulates one full
Interpreterallocation per failure until process exit. Not a crash, but the PR's stated goal is to make this error path safe instead of aborting, and unconditionally leaking the interpreter on every such error is a concrete defect in that fix.How to fix
Route
Yield::Failedthrough a shared teardown that knows whether async work is actually in flight — the same fix already proposed in the earlier thread for the async-reentry hang. Concretely: after the trampoline returns with a pending exception, walknodesfor any liveExec::Subprocand kill/detach it (nulling itsinterpbackref), then decrementhas_pending_activity. In the non-pipeline case that walk finds nothing and the decrement is safe; in the pipeline case the subprocess is torn down first so the decrement is also safe. A narrower alternative for this call site alone: decrement only when no node holds a liveExec::Subproc, and leave the pipeline case pinned as-is — but that still leaves the async-reentry hang from the earlier thread, so the shared path is the better investment.
…cess is live reject_pending_exception now walks the node arena for any Cmd with a live Exec::Subproc and only decrements the GC-pin counter when none is found, so the common throw-before-spawn case no longer leaks the Interpreter box while the pipeline-sibling case stays pinned against UAF.
|
Addressed in 82c82c3. A full teardown (kill/detach every live subprocess before decrementing) would let the pipeline case collect too, but that touches the subprocess lifecycle enough that it belongs with #30550 or a dedicated follow-up. |
…ines Continuing to drain after the promise was rejected started downstream commands whose PipeReader could outlive the interpreter (UAF when the failing command was first) or re-entered reject_pending_exception with the callback already cleared (spurious uncaught error when both sides threw). Returning immediately after reject leaves unspawned pipeline siblings unstarted; already-spawned ones stay pinned by the has_live_subproc check. Adds first-in-pipeline and both-sides test cases.
|
CI status: the shell changes are green on all lanes across builds 72050, 72080, 72092, and 72113 (
Yellow retries (napi GC timing, spawn timeout, bake HMR, install migration/registry, repl EPIPE, http-connect, zlib leak, serve-file, no-orphans, bun-run-dir) are pre-existing Windows/aarch64/asan flakes and passed on retry. Ready for review. |
…Failed
The Exec::Subproc scan missed other raw *mut Interpreter backref holders
(ShellGlobTask, builtin ShellTask, ShellCondExprStatTask), so
ls * | cmd < ${stream} decremented to 0 while a glob task was still on
the threadpool and UAF'd when it bounced back. A sound predicate would
need to enumerate every such holder; leave the GC pin in place on this
path (one Interpreter leaked per failed invocation) until per-backref
refcounting lands.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/shell/interpreter.rs:1398-1399— This comment narrates the absence of the deletedif global_this.has_exception()block, which src/CLAUDE.md prohibits ("NEVER add comments to deleted code blocks"), and the root CLAUDE.md says bug history belongs in the PR description — where this rationale already is. Delete the two lines, or if you want to keep something, trim it to just the invariant ("Yield::runrejects the promise onYield::Failed") without the "no post-run exception check is needed here" clause.Extended reasoning...
What the issue is
The diff replaces the deleted post-run exception check with a comment explaining why it was removed:
Script::start(self, root).run(self); // `Yield::run` rejects the promise and releases keepalive on // `Yield::Failed`, so no post-run exception check is needed here.
Two repo conventions apply:
- src/CLAUDE.md line 21: "NEVER add comments to deleted code blocks."
- Root CLAUDE.md (Code style & idioms): "Comments carry only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications, deliberate deviations from upstream. No narrating what the code does, no bug history — that belongs in the PR description."
Why this comment falls under the rule
Walk through what a reader without git history sees. Before this PR, lines 1397-1399 were:
if global_this.has_exception() { return Err(crate::jsc::JsError::Thrown); }
The diff deletes that block and inserts, in the same position, a comment whose second clause is "so no post-run exception check is needed here." That phrase only makes sense as a reference to the check that used to be there — it's answering "why isn't there an exception check here?", which is a question a reader only asks if they know one used to exist. That is exactly the pattern src/CLAUDE.md's rule targets: a comment standing in for deleted code, recording the fix history rather than a durable invariant of the surrounding code.
Why the rule exists here
The rationale ("the trampoline now handles
Yield::Faileditself") is genuine, but it's already stated in three places that outlive it better than an inline comment: the PR description's Fix section, the doc comment onInterpreter::reject_pending_exceptionat interpreter.rs:1259-1261, and the inline comment inside theYield::Failedarm at Yield.rs:143-145. A future reader ofrun_from_jswho hasn't seen this diff has no reason to expect an exception check after.run(), so the comment answers a question nobody will ask — and per the root CLAUDE.md, that history belongs in the PR description, not the code.Impact
None functionally — this is purely a style/convention nit with no runtime effect.
How to fix
Delete the two comment lines:
Script::start(self, root).run(self); - // `Yield::run` rejects the promise and releases keepalive on - // `Yield::Failed`, so no post-run exception check is needed here. Ok(crate::jsc::JSValue::UNDEFINED)
If you want to keep a pointer for readers, trim it to the invariant only — e.g. "
Yield::runrejects the ShellPromise onYield::Failed." — dropping the "so no post-run exception check is needed here" clause that references the deleted code.
A second Yield::Failed reached from a separate .run() invocation (IOWriter::on_error loops call run_yield per child) would re-enter reject_pending_exception with the reject callback already cleared and report the follow-up exception as unhandled. Gate on a rejected flag and swallow subsequent exceptions once the promise has settled. Also drop the comment in run_from_js that referenced the removed post-run exception check.
|
Re: the |
Moving the check into parse_atom() covers every caller: parse_simple_cmd,
both parse_assign value arms (FOO=${resp} previously said 'Expected an
atom'), and the [[ ]] operand path. parse_redirect consumes JSObjRef via
r#match before calling parse_atom, so valid redirects are unaffected.
Reworded the message to name the redirect operators rather than a concrete
example, since '> ${value}' is rejected for Blob/Response and '< ${value}'
with a ReadableStream currently panics (pre-existing, #33996).
Added positive assertions for the assignment-value, glued-value, [[ ]]
operand and [[ ]] operator positions across all five types.
1 participant
Passing a
ReadableStreamas a redirect target in Bun Shell aborts the whole process when the command resolves to a spawned subprocess:The builtin path (
echo hi > ${stream}) already threwUnknown JS value used in shell: [object ReadableStream]. The subprocess path inCmd::init_subproc_redirectionsinstead had a literalpanic!("TODO SHELL READABLE STREAM")that ran before any direction check, so every direction (<,>,2>) aborted. On POSIXcatandcpareposix_disabledbuiltins, so they always take the subprocess path; any spawned command hits it too.Fix
src/runtime/shell/states/Cmd.rs: drop theReadableStreamplaceholder branch so the value falls through to the existingUnknown JS value used in shellthrow, matchingBuiltin::init_redirections.src/runtime/shell/Yield.rs,src/runtime/shell/interpreter.rs: onYield::Failedthe trampoline now callsInterpreter::reject_pending_exceptionand returns immediately instead of draining pipelines. That method takes the pending exception, rejects the ShellPromise with it, and releases the event-loop keepalive. It is guarded by arejectedflag onInterpreterFlagsso a secondYield::Failedreached from a separate.run()invocation (e.g. anIOWritererror loop) swallows the follow-up exception instead of reporting it as unhandled.has_pending_activityis left pinned because a pipeline sibling's subprocess / glob task / builtin threadpool task may still hold a raw*mut Interpreter.src/js/builtins/shell.ts: the cachedrejectcallback was never invoked from native; repurpose it to forward the error value straight to the promise reject so the caller'scatchreceives the thrown error.Known limitation
spawned-cmd | throwing-cmd(a pipeline sibling that already spawned before the throw) leaves the interpreter GC-pinned and the sibling's pipe read-end held open. If the sibling writes more than the socketpair buffer (~64KB) it blocks inwrite(2)and the process hangs after thecatchfires. The builtin variant of this already hung onmainpre-PR. Resolving it soundly needs a detach-then-release walk (close unwrapped pipe ends, kill/detach live subprocesses, cancel in-flight shell tasks, then release the pin), which belongs in a follow-up alongside #30550.Verification
8 cases added to
bunshell.test.ts(stdin / stdout / stderr to subprocess,Response.body, pipeline withBun.gc(true), first-in-pipeline, both-sides-of-pipeline,&&async re-entry). Each spawns a child that catches the error and asserts a clean exit.Full
bunshell.test.tsis green (394 pass, 0 fail).Related: #18262. #30550 is the full implementation of
ReadableStreamas shell stdin; this change is the minimal safety fix so the placeholder no longer crashes the process in the meantime.