PR #37879 - oven-sh/bun

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 #37703 and #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 #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 #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 #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.

#37879 spawn: release the stdio PipeReader through its pointer, not a &mut receiver

Problem

Fix

Background

Problem

Fix

Tests

Verification

spawn: release the stdio PipeReader through its pointer, not a &mut receiver