shell: fix use-after-free when the poll re-registration issued from PipeReader::on_read_chunk fails - #33269
Conversation
|
Updated 4:20 PM PT - Jul 2nd, 2026
❌ @robobun, your commit a769948 has some failures in 🧪 To try this PR locally: bunx bun-pr 33269That installs a local version of the PR into your bun-33269 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR removes explicit re-arming of polling/reading from ChangesPipeReader re-arming fix and fault-injection test
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
register_poll()'s failure path dispatches on_reader_error, which drops the last Arc<PipeReader>. Calling it from inside on_read_chunk (which runs from the bun_io read loop, with &mut to the embedded PosixBufferedReader live on that loop's stack) therefore frees the reader the loop keeps reading through: a heap use-after-free in read_with_fn's streaming inner loop whenever a single poll wake drains more than half the 256 KB scratch buffer and the re-registration's epoll_ctl(MOD) fails. The re-arm is also redundant: the POSIX read loop re-registers the poll itself based on the bool on_read_chunk returns, and on Windows WindowsBufferedReader::on_read / on_file_read handle it. The sibling shell reader (IOReader::on_read_chunk_cb) already dropped its re-arm for the same reasons.
37e801f to
435e4be
Compare
|
Removed the two stray |
There was a problem hiding this comment.
Thanks for cleaning up the stray output files — the diff is back to the two intended changes. The fix looks sound and matches the sibling IOReader::on_read_chunk_cb pattern, but since it removes re-arm behavior on both the POSIX and Windows unsafe read paths, it's worth a human sign-off.
Extended reasoning...
Overview
The PR deletes the redundant register_poll() / start_with_current_pipe() re-arm from shell PipeReader::on_read_chunk in src/runtime/shell/subproc.rs (plus the now-unused Windows Output import), and adds a SHELL_RECV_BULK fault-injection mode with a new ASAN-gated regression test in test/js/bun/shell/shell-pipe-read-fault.test.ts. The two stray *_output.txt files I flagged in my previous review have been removed in 435e4be, and the current diff is scoped to exactly these two files.
Security risks
None identified. This is an internal I/O lifetime fix in the shell subprocess reader; it does not touch auth, input validation, network protocol parsing, or any user-facing trust boundary. The test changes only extend an existing LD_PRELOAD fault-injection shim that runs inside a temp dir under the test harness.
Level of scrutiny
High. The change sits in memory-safety-critical unsafe Rust: raw *mut PipeReader pointers, Arc strong-count juggling, and a documented invariant ("on_read_chunk never frees the reader") that the deleted code was violating transitively. I verified the load-bearing claims against the codebase — src/io/PipeReader.rs:404 documents register_poll() -> bool's failure path dispatching on_reader_error, src/io/PipeReader.rs:628/820 document the on_read_chunk-never-frees contract, and src/runtime/shell/IOReader.rs:299-304 shows the sibling on_read_chunk_cb already dropped the identical re-arm with the same rationale. The reasoning is internally consistent and the ASAN regression test is deterministic.
That said, the deletion also removes the Windows start_with_current_pipe() call (whose Output::panic("TODO: ...") error path is being retired). The PR asserts WindowsBufferedReader::on_read already handles both the re-arm and the _buffer.clear() side effect, which I did not independently trace end-to-end. Given the cross-platform reach and the fact that correctness here hinges on subtle re-registration ownership across the bun_io read loop, this is exactly the kind of change a maintainer familiar with the PosixBufferedReader / WindowsBufferedReader state machines should sign off on.
Other factors
- No bugs surfaced by the bug-hunting sweep on the current revision.
- My only prior feedback (stray
append_output.txt/pwd_output.txt) was addressed and the thread is resolved. - The PR description is unusually thorough (ASAN traces, freed-by stack, before/after test evidence, and a shell-suite sweep confirming no new regressions), which raises confidence but doesn't substitute for maintainer review of an
unsafe-heavy I/O path.
|
Thanks for the review. The Windows half is the part I could not exercise here, so here is the trace for whoever signs off: On Windows, shell
That comment block was written when the sibling shell reader, On the red CI check: the only failed job on this build so far is |
CI status for 435e4be282 jobs passed, including the Two jobs are red, both on darwin, and neither is reachable from this diff: 1. 2. That test drives I am not pushing a retrigger commit: that terminal timeout is deterministic (identical 90 s timeout on all four attempts), so a re-run would reproduce it rather than clear it. It also looks latent rather than new, which is probably why it is not showing up elsewhere: on the four most recent finished PR builds I checked (68037, 68040, 68041, 68042), the darwin Happy to rebase or dig into the terminal lane separately if a maintainer would like. |
|
Correction to my previous comment, and a retrigger. Build 68021 has now finished, and the darwin lanes it did not get to earlier changed the picture. So It also means I was wrong to call the failure deterministic: all four retries ran inside the same job on the same agent, so they tell us nothing about a fresh one. I have pushed a single Final tally for 435e4be was 284 passed, 2 failed:
Both darwin 14 aarch64 shards and the sibling shard of each failing lane passed, and the |
Status: diff is green, CI is red on unrelated darwin infra. Needs a maintainer.I have used my one retrigger (a769948) and will not push another. Summary of where this stands, so nobody has to re-derive it. The change is verified
The two red jobs, neither reachable from this diff
This is
This change is arch-independent, so a test that passes on aarch64 and hangs on x64 from the same build is not caused by it. Mechanically it also cannot be: that test drives AskThe code is unchanged since 435e4be and |
Retrigger result: everything that ran tests is green. The only red is one broken CI agent.Build 68070 (on a769948) finished 284 passed, 2 failed, and the two failures are both the same Buildkite agent failing to download the build artifacts before it ever starts a test. The failure is an agent, not a lane and not this diffComparing the
Every job that lands on Every job on any other agent passes. On 68070 both shards of that lane happened to land on the bad agent, which is why the lane is fully red this time. This looks worth a separate look by someone with infra access; it is not something a PR can fix. The previous darwin 14 x64 failure did not recur
The change is verifiedAll 20 AskThe code is unchanged since 435e4be, |
2 participants
Problem
Heap use-after-free in Bun Shell when
epoll_ctl(MOD)fails while the shellPipeReader::on_read_chunkcallback re-registers the poll from inside the read loop. Found by syscall-fault-injection fuzzing againstorigin/main.This is the path #32986 called out as out of scope: that PR fixed
read_with_fn's ownEAGAIN-arm re-registration, but the shellPipeReader::on_read_chunkstill calledself.reader.register_poll()itself.Freed-by stack (the re-entrant callback chain)
Repro
FilePollfires and__bun_run_file_polldispatches intoPosixBufferedReader::on_poll->read_with_fnwith a bare&mutand no keepalive.recv()drains more than half of the 256 KB scratch buffer in one call, soread_with_fn's streaming inner loop flushes the head mid-loop:parent.vtable.on_read_chunk(.., Progress).PipeReader::on_read_chunkre-arms the poll itself:self.reader.register_poll(). Theepoll_ctl(MOD)fails (ENOMEMin the repro; fd/watch pressure in the wild).register_polldispatcheson_reader_error. The shellPipeReader::on_reader_errorsignals theCmdand drops theReadable::PipeArc; its ownguard_from_rawkeepalive becomes the last reference, and dropping it frees thePipeReader(and thePosixBufferedReaderembedded in it).register_pollreturnsfalse, buton_read_chunkis not a direct caller of the read loop, so thefalsenever reaches it. The inner loop keeps going and readsparent._offsetfrom the freed reader on the nextrecv.Cause
BufferedReaderParent's contract (and theSAFETYcomments inread_with_fn/read_blocking_pipe) is thaton_read_chunknever frees the reader; onlyon_reader_errormay. The shellPipeReader::on_read_chunkbroke that transitively by callingregister_poll(), whose failure path dispatcheson_reader_error.#32986's
register_poll() -> boolreturn value only protects direct callers in the read loop. It cannot protect a caller that reachesregister_pollthrough theon_read_chunkvtable dispatch two frames down.Fix
Delete the re-arm from shell
PipeReader::on_read_chunk. It was redundant on both platforms and the codebase already documents why:read_with_fn/read_blocking_pipethat wants more data already callsregister_poll()itself, driven by theboolon_read_chunkreturns.WindowsBufferedReader::on_readnotes "the re-arm is already handled byon_file_read's epilogue /uv_read_start", and it already performs the_buffer.clear()that used to bestart_with_current_pipe()'s second side effect.IOReader::on_read_chunk_cb, already dropped its identical re-arm for the same two reasons (redundancy, plus re-deriving&mutto the embedded reader while the read loop holds one).Removing it also removes the only
&mut self.readerre-derivation inside the callback, and theOutput::panic("TODO: ...")that was the Windows branch's only error handling.Test
New
SHELL_RECV_BULK=Nmode intest/js/bun/shell/shell-pipe-read-fault.test.ts'sLD_PRELOADshim: the first N realrecv()s on eachAF_UNIXsocket instead return the caller's whole buffer filled with'A'. Combined with the existingSHELL_RECV_EAGAIN_FIRST=1andSHELL_FAIL_EPOLL_FROM=3, one fabricated bulk recv deterministically pusheshead_startpast the half-buffer cutoff so the mid-loop flush (and therefore the failing re-registration) happens fromon_read_chunk.With the epoll failure count unchanged, the same
epoll_ctl#3 that used to be issued byon_read_chunkis now the read loop's ownEAGAINre-registration, whose failure path already returns without touching the reader, so the command just reportsENOMEM.heap-use-after-freeabove; the other 6 tests in the file pass.The test is
skipIf(!isASAN)like its sibling.Also ran the rest of
test/js/bun/shell/(bunshell*.test.ts: 394 pass / 0 fail;commands/and the remaining files: every failure reproduces identically withsrc/runtime/shell/subproc.rsreverted tomain, so they are pre-existing in this environment, not caused by this change).