spawn: release the stdio PipeReader through its pointer, not a &mut receiver by robobun · Pull Request #37879 · oven-sh/bun

robobun · 2026-08-12T16:26:12Z

Problem

No crash is known from this; nothing touches the memory after the free. What is wrong is the aliasing contract on a path that runs at the end of every Bun.spawn, spawnSync and child_process stdout or stderr pipe. Same class as fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703 and io: report to pipe writer parents through raw pointers, release StaticPipeWriter through its backref #37755.
While a read is in flight the stdio reader holds two refs, one owned by the Subprocess and one taken at start. The done and error callbacks ask the Subprocess to drop its ref, then release their own through the &mut self receiver, so the release that normally frees the reader runs while a protected borrow of it is still live.
One line earlier the same callback has the Subprocess reach into the reader through its own pointer to collect the buffered output, also while the receiver is live.
A standalone reduction of this shape fails under Miri in both models (Tree Borrows: reborrow through <475> at alloc296[0xc] is forbidden; Stacked Borrows: not granting access to tag <490> because that would remove [Unique for <1841>] which is strongly protected) and passes with the shape used here.

Fix

The functions that can end the reader's life (done, error, the first read, and the failure path inside start) take the reader's raw pointer instead of &mut self. Done and error share one tail: adopt the start ref into a scoped guard, record the state, ask the Subprocess to drop its ref, and let the guard release when it drops.
Property to check: no reference to the reader is live when the Subprocess reaches back into it or when the last ref is released; each &mut still formed lasts one statement and ends before either happens. No refs are added or removed, only the pointer the existing releases go through changes.
Left as is on purpose: the io layer's own &mut self start beneath the failure path, plus close and watch (follow-ups noted at their sites since Make re-entrant runtime objects &self-only; delete AnyTask #36571), and the detach in to_js, whose release is never the last one.
Verification: a new source lint bans releasing through the receiver; it reports exactly the two old lines against main and nothing against this branch. The spawn and child_process suites pass on debug ASAN and Windows x64 debug builds, including the tests that fault-inject a failed start and a read error. Miri was run on the reduction only, not on bun.

Background

Intrusive refcount: the reader carries its own count. deref decrements it, and the decrement that reaches zero runs the destructor and frees the allocation, so any given release may be the last one.
Protectors: under Stacked Borrows and Tree Borrows (the aliasing models Miri checks), a reference passed as a function argument, &mut self included, is protected for the whole call. Freeing its target, or touching it through another pointer, is undefined behavior during the call even if the reference is never used again; a raw *mut Self argument creates no protector.
The reader's two refs: the Subprocess holds one through its Readable::Pipe slot and start takes a second for the in-flight read. on_close_io is the Subprocess taking the buffered output and dropping its slot ref; the callback then releases the start ref, which is why that release normally frees the reader.
Vtable dispatch: the io-layer buffered reader hands its parent a *mut Self on done and error; a macro line per parent decides whether the method receives that pointer or an autoref'd &mut. The layers below were already made raw for this reason in Make re-entrant runtime objects &self-only; delete AnyTask #36571.
ScopedRef: a guard over a refcounted pointer. new takes an extra ref and releases it on drop (a keepalive); adopt takes over a ref the caller already holds and releases it on drop, which places the release after everything else in the scope.

Original description

Problem

PipeReader::on_reader_done(&mut self) and on_reader_error(&mut self) in src/runtime/api/bun/subprocess/SubprocessPipeReader.rs (lines 244 and 359 on main) both ended with

unsafe { PipeReader::deref(self) };

The reader has two refs while a read is in flight: the one Readable::Pipe holds and the one start() takes. A line earlier, process.on_close_io(kind) has made the Subprocess drop the Readable::Pipe ref, so this deref is normally the last one and deinit frees the allocation while the &mut self receiver is still live. A reference passed as a function argument is protected for the whole call, and both aliasing models reject freeing protected memory, whether or not self is used again afterwards. The same function also breaks the protector before that point: on_close_io reaches back into the reader through the Readable's own pointer (Readable::pipe_reader_mut(&pipe).state, to take the buffered output) while the receiver that just wrote state is live, which is a foreign access to protected memory.

The chain below these two functions is already raw: PosixBufferedReader::done/on_error/read/register_poll take *mut precisely so that "the (maybe-freeing) dispatch runs under no receiver protector" (#36571, and the comment on the inject_stdio_read_error testing hook says the same), and the vtable hands the parent a *mut Self. The macro line on_reader_done = |this| (*this).on_reader_done(); then re-introduced the protected &mut for the one hop that does the freeing. This runs at the end of every Bun.spawn / spawnSync / child_process stdout or stderr pipe; there is no known crash from it (nothing touches the memory after the free), it is the contract that is wrong, same as #37703 (FetchTasklet) and #37755 (StaticPipeWriter).

A standalone reduction of exactly this shape (owner holding one ref, the callback asking the owner to drop it and then releasing its own through the receiver) fails under Miri with both models, and passes with the shape this PR uses:

# main's shape, Tree Borrows (the model `bun run rust:miri` uses)
error: Undefined Behavior: reborrow through <475> at alloc296[0xc] is forbidden
   |         let state = unsafe { &mut (*reader).state };        <- on_close_io
   = help: the accessed tag <475> is foreign to the protected tag <1799>
   = help: protected tags must never be Disabled
help: the protected tag <1799> was created here
   |     fn on_reader_done_before(&mut self) {

# main's shape, Stacked Borrows
error: Undefined Behavior: not granting access to tag <490> because that would remove [Unique for <1841>] which is strongly protected

(With the owner's access removed, the trailing release alone fails at the deallocation instead; that variant is the reduction in #37703.)

Reduction

use std::cell::Cell;

struct Reader { ref_count: Cell<u32>, owner: *mut Owner, state: u32 }
struct Owner { slot: Option<*mut Reader> }

impl Owner {
    // Subprocess::on_close_io: drop the Readable's ref, taking the output first.
    fn on_close_io(&mut self) {
        let reader = self.slot.take().unwrap();
        let state = unsafe { &mut (*reader).state };
        *state = 0;
        Reader::deref(reader);
    }
}

impl Reader {
    fn deref(this: *mut Reader) {
        let n = unsafe { (*this).ref_count.get() } - 1;
        unsafe { (*this).ref_count.set(n) };
        if n == 0 { drop(unsafe { Box::from_raw(this) }); }
    }
    // main: the vtable autoref'd `&mut *this`.
    fn on_reader_done_before(&mut self) {
        self.state = 1;
        unsafe { (*self.owner).on_close_io() };
        Reader::deref(self);
    }
    // this PR
    unsafe fn on_reader_done_after(this: *mut Reader) {
        let owner = unsafe { (*this).state = 1; (*this).owner };
        unsafe { (*owner).on_close_io() };
        Reader::deref(this);
    }
}

fn main() {
    let owner = Box::into_raw(Box::new(Owner { slot: None }));
    let reader = Box::into_raw(Box::new(Reader { ref_count: Cell::new(2), owner, state: 0 }));
    unsafe { (*owner).slot = Some(reader) };
    if std::env::args().nth(1).as_deref() == Some("after") {
        unsafe { Reader::on_reader_done_after(reader) };
    } else {
        unsafe { (*reader).on_reader_done_before() };
    }
    drop(unsafe { Box::from_raw(owner) });
}

cargo miri run -- before and MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- before report the errors quoted above; -- after exits 0 under both.

Fix

The functions that can end the reader's life take the pointer the reader registered as its parent, and release through it:

on_reader_done(this) / on_reader_error(this, err) share a tail, finish(this, state): it adopts the start() ref into a bun_ptr::ScopedRef first, records the state and takes the process backref through statement-scoped field accesses, calls on_close_io with no borrow of the reader live, and the guard releases when it drops. The order of operations (state, on_close_io, release) is the one the two functions had; the only &mut formed is the call-scoped one for to_owned_slice(), which is over before on_close_io runs. kind() takes the address it compares instead of &self, so nothing in this tail forms a reference to the reader. The vtable lines forward this, as the shell's reader in src/runtime/shell/subproc.rs already does.
read_all(this): the spawn bindings call it right after start(), and EOF on that first read reaches on_reader_done synchronously (the io-layer read is already raw), so its &mut self was the one protected frame on the spawnSync / non-lazy spawn path.
start(this, ..): when registering the pipe fails, the teardown runs inside start() and the keepalive guard's drop at the end of start() is the final release. The guard is now built from this (it was ScopedRef::new(ptr::from_mut(self))), and the Windows arm calls on_reader_error(this, err). The io layer's own start(&mut self) over the reader field is still beneath the dispatch on this path; that receiver is listed as a follow-up at its site in src/io/PipeReader.rs (from Make re-entrant runtime objects &self-only; delete AnyTask #36571), along with close/watch, which is why PipeReader::close()/watch() are left as they are here.
The two call sites in spawn_maybe_sync pass pipe.as_ptr() and re-read the Readable slot between start() and read_all() (they already did, since a failed start replaces the slot). to_js's detach is not touched: its release is never the last one, because Readable::to_js still holds the slot's ref and releases it afterwards in pipe_detach.

No refs are added or removed; the diff changes which pointer the existing releases go through.

Tests

test/internal/source-lints/self-receiver-deref.test.ts bans a release from the deref family, or ScopedRef::adopt, applied to the receiver itself (deref(self), deref_with_context(self, ..), deref(&mut *self)), which is the spelling the lint in #37703 does not cover (that one looks for a pointer spelled out from a reference, deref(ptr::from_mut(self)) and friends). It checks its pattern against positive and negative examples, skips fn deref(self) items and Deref::deref(self), and ratchets the other instances in the tree at their current counts with the reason for each (h2 ClientSession::on_close/maybe_release, h3 ClientSession::detach, ProxyTunnel::detach_and_deref; these are the same shape and are to be converted separately, the entries keep them from multiplying). With src/ at main it reports exactly

src/runtime/api/bun/subprocess/SubprocessPipeReader.rs:244
src/runtime/api/bun/subprocess/SubprocessPipeReader.rs:359

and passes with this branch; the rest of test/internal/source-lints/ still passes.

Verification

On the debug (ASAN) build: test/js/bun/spawn/spawn.test.ts (140 pass, 6 skip); spawnSync, spawn-pipe-read-error-leak (both the epoll variant and the injectStdioReadError one, which drive on_reader_error through the raw io entry), spawnsync-isolated-event-loop, spawn-unread-stdout-gc, spawn-stdout-iterate-leak, spawn-stdout-filereader-gc-uaf, spawn-noread-leak, spawn-maxbuf, spawn-many-teardown, spawn-streaming-stdout, exit-code, spawn-pipe-stale-fd-unregister, readablestream-helpers (80 pass, 3 skip); test/js/node/child_process/{child_process,child-process-stdio,child_process-node,child-process-exec} (110 pass). The three child_process failures in that run are this container's: $SHELL is unset ("should allow us to spawn in the default shell"), "it accepts stdio passthrough" passes when run alone, and the fixture of "extra stdio pipes are not double-closed on GC" (stdout/stderr ignore, so no pipe reader is involved) prints OK in about 6 s on this build against the test's 5 s limit. cargo check -p bun_runtime for the host and for x86_64-pc-windows-msvc, cargo clippy -p bun_runtime and cargo fmt --check are clean.

On a Windows x64 debug build of this branch: test/js/bun/spawn/spawn-pipe-start-error.test.ts (Windows debug only; it fault-injects the start_with_current_pipe failure, so it is the test of the Windows arm of start() and of the keepalive's drop being the final release) and spawn-pipe-read-error-leak.test.ts (the injected variant, which enters on_reader_error from the libuv reader) pass; spawn.test.ts (124 pass, 19 skip, 3 todo) and spawnSync, spawn-unread-stdout-gc, spawn-maxbuf, spawn-streaming-stdout, exit-code (26 pass, 13 skip) pass as well.

…eceiver PipeReader::on_reader_done/on_reader_error ended with PipeReader::deref(self). on_close_io has already dropped the Readable's ref by then, so that deref is normally the reader's last one and frees the allocation while the &mut self receiver is still a live, protected argument. on_close_io also reaches back into the reader through the Readable's own pointer while that receiver is live. Both callbacks now take the parent pointer the BufferedReader registered and share a tail (finish) that adopts the start() ref into a ScopedRef, records the state and calls on_close_io through statement-scoped field accesses, and releases once those have ended. start() and read_all(), which can reach the same teardown synchronously (a failed registration, EOF on the first read), take the pointer as well; the spawn bindings pass the Readable's pointer and re-read the slot in between. kind() compares addresses only. Adds a source lint for the bare deref(self) / ScopedRef::adopt(self) spelling, with the remaining http instances ratcheted at their current counts.

coderabbitai · 2026-08-12T16:26:22Z

Warning

Review limit reached

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

Next review available in: 1 minute

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: 3551f559-9f7b-40f2-a058-4d5c1e8112a2

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 0f7364b.

📒 Files selected for processing (3)

src/runtime/api/bun/js_bun_spawn_bindings.rs
src/runtime/api/bun/subprocess/SubprocessPipeReader.rs
test/internal/source-lints/self-receiver-deref.test.ts

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

robobun · 2026-08-12T16:27:30Z

Status: reproduced as the source shape at src/runtime/api/bun/subprocess/SubprocessPipeReader.rs:244 and :359 on main (the new lint lists exactly those two lines with src/ at main; a Miri reduction of the shape is in the description). Fix and lint are in this PR. The Windows arm of start() was run on a Windows x64 debug build of this branch (spawn-pipe-start-error.test.ts, spawn-pipe-read-error-leak.test.ts, spawn.test.ts, spawnSync.test.ts all pass; details in the description).

robobun · 2026-08-12T16:35:23Z

^{Updated 3:05 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 0f7364b has 2 failures in Build #93503 (All Failures):

/opt/winsysroot/Windows Kits/10/Include/10.0.26100/um/winver.h - clang warning on 🪟 x64 - build-bun
scripts/build/ci.ts - build failed on 🍎 x64 - build-bun
scripts/build/ci.ts - build failed on 🐧 x64-asan - build-bun

🧪 To try this PR locally:

bunx bun-pr 37879

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

bun-37879 --bun

claude

I reviewed this PR and didn't find any bugs. Because it reworks the unsafe refcount-release path that runs at the end of every Bun.spawn stdout/stderr pipe, a human look would still be worthwhile.

What was reviewed:

ScopedRef::adopt semantics (src/ptr/ref_count.rs:992) — takes ownership of an existing ref without incrementing, so finish correctly consumes the start() ref rather than adding one.
Refcount balance in finish: order (state → on_close_io → guard drop) matches the old on_reader_done/on_reader_error, and no &mut *this outlives the statement it's formed in.
The vtable forwarding matches the established pattern in src/runtime/shell/subproc.rs:2230; the lint's ALLOW counts match the current tree (h2: 2, h3: 1, ProxyTunnel: 1).

Extended reasoning...

Overview

This PR converts PipeReader::on_reader_done, on_reader_error, start, read_all, and kind in src/runtime/api/bun/subprocess/SubprocessPipeReader.rs from &mut self receivers to raw *mut Self, and updates the two call sites in js_bun_spawn_bindings.rs to pass pipe.as_ptr(). The motivation is a Stacked/Tree Borrows aliasing-model violation: the old on_reader_done(&mut self) ended with PipeReader::deref(self), which frees the allocation while the receiver's protector is still live. A new shared finish(this, state) helper adopts the start() ref into a ScopedRef guard so the release goes through the raw pointer after all borrows have ended. A source-lint test ratchets the remaining deref(self) instances in src/http/.

Security risks

None. This is an internal memory-model correctness fix with no user-facing API surface, no parsing of untrusted input, and no auth/crypto involvement.

Level of scrutiny

High. Per REVIEW.md this is the "most-blocked category" — native memory safety with intrusive refcounting on a path that runs at the end of every Bun.spawn/spawnSync/child_process piped stdio. The change is not mechanical: it rewrites ~150 lines of unsafe across cfg(windows)/cfg(unix) branches, and the correctness argument depends on the distinction between ScopedRef::new (takes a new ref) and ScopedRef::adopt (takes ownership of an existing one), plus the drop ordering of the _start_ref guard relative to on_close_io. I verified ScopedRef::adopt at src/ptr/ref_count.rs:992 does not increment, so finish releases exactly the ref start() took (matching the old explicit deref(self)), and confirmed the pattern mirrors src/runtime/shell/subproc.rs:2223-2231 which already forwards the raw *mut Self for the same reason.

Other factors

The PR description is unusually thorough — it includes a standalone Miri reduction that fails under both aliasing models on the old shape and passes on the new one, and reports passing the full spawn/child_process suite on a debug ASAN build plus cargo check for both host and x86_64-pc-windows-msvc. The lint's ALLOW list matches what I found by grepping src/http/ (2 in h2 ClientSession, 1 in h3 ClientSession, 1 in ProxyTunnel). The sibling lints referenced (self-receiver-reclaim.test.ts, fn-long-mut-reborrow.test.ts) and the globAllSources helper all exist. Nothing looks wrong, but the refcount-balance and Windows-arm reasoning are subtle enough that a maintainer familiar with #37703/#37755 should confirm.

…tching instead of with lookbehinds With the lookbehinds at the head of the pattern the scan of the tree took about 6 s; matching the release token first and checking the preceding text brings it under 0.1 s, in line with the sibling lints.

github-actions · 2026-08-12T17:44:07Z

+    // Both calls go through the reader's own pointer because either may end
+    // the reader's life before returning (a failed start, or EOF inside
+    // read_all), at which point on_close_io has already replaced the slot, so
+    // the slot is re-read in between instead of reusing `pipe`.
