io: report to pipe writer parents through raw pointers, release StaticPipeWriter through its backref

RO
3mo ago
+726-318

Problem

  • Draining a buffer or Blob into a child's stdin (Bun.spawn when memfd is not used, always on macOS, and every shell $ command with a buffer on stdin) frees the StaticPipeWriter from inside its own on_write(&mut self): the owner drops its ref during writer.close(), so the trailing deref is normally the last one.
  • Every frame below it, from the poll dispatch down to the vtable shim, held the embedded writer by &mut too, and that writer is a field of the object being freed. Freeing an allocation while a reference into it is a live argument is UB under both Rust aliasing models, even if nothing touches it afterwards.
  • A reduction of the chain fails under Miri: error: Undefined Behavior: deallocation through <1716> at alloc239[0x0] is forbidden (Tree Borrows), and at the Box::from_raw under Stacked Borrows. FileSink and the install security scanner's writer release through the same chain.
  • No crash is known from this: the free is the last thing any of these frames do. The contract is what is wrong, the same one #36571 moved PipeReader off.

Fix

  • The POSIX poll chain passes the writer as *mut end to end: each frame takes &mut only for statement-scoped work, and the report or close that may free the parent is its last use of the pointer (a raw close entry delivers on_close with nothing borrowed). Property to check: at every call that may free the parent, no reference into the parent or its writer is live.
  • StaticPipeWriter moves to the raw dispatch mode and the &mut mode is deleted from both parent macros (it was the only user). Its on_write takes a ref of its own across writer.close() when start()'s is already gone (the scanner) and releases through the raw pointer last; on EOF it no longer closes the writer itself, which used to leak it. The subprocess owners stop borrowing the whole writer from on_close_io and only drop their ref.
  • Verification: nothing observable changes, so no test fails without the fix. A new source lint rejects main (3 hits) and passes here; the Miri reductions pass once converted; refcount logs on the ASAN debug build show Bun.spawn, the shell and bun pm scan ending in the same order; a new scanner test covers the EPIPE path (passes before and after); the spawn, shell, filesink and security-provider suites pass under ASAN; Windows cargo check passes.
  • Left alone on purpose: the zero-drain error path still leaks start()'s ref on POSIX (pre-existing, tracked separately), and the partial-drain error is still reported under &mut self (that is the #34697 protocol change).

Background

  • StaticPipeWriter writes one fixed buffer or Blob into a spawned child's stdin; Bun.spawn, the shell and bun pm scan's security scanner all use it. It is refcounted: create() hands the owner a ref, start() takes another, and whoever drops the last one frees it.
  • PosixBufferedWriter is the generic fd-draining machinery, embedded as a field of that parent with a raw backref to it. It reports on_write (Pending, Drained or EndOfFile), on_error and on_close through a vtable the impl_*_writer_parent! macros generate; the macro's borrow mode decides whether the parent's handlers get &*this, &mut *this or the bare *mut.
  • Because the writer is a field of the parent, freeing the parent frees the writer, so a frame entered through &mut writer still points into memory the parent's handler may free.
  • Stacked Borrows and Tree Borrows (the aliasing models Miri checks) protect a reference for as long as it is an argument of a running function: deallocating what it points into during that time is UB whether or not the reference is used again. Raw pointers have no protector, which is why *mut plus short reborrows is the sound shape.
  • on_close_io is the owner's callback for one of its stdio pipes closing; for stdin it is where the owner drops its create() ref, which is why the final free normally lands inside writer.close().
Original description

Problem

StaticPipeWriter::on_write(&mut self, ..) (src/spawn/static_pipe_writer.rs) ended with

unsafe { RefCount::<Self>::deref(std::ptr::from_mut::<Self>(self)) };

releasing the ref start() took. Just before it, self.writer.close() runs on_close -> P::on_close_io, where the owner (Subprocess, ShellSubprocess) drops the ref it got from create(), so on POSIX that release is normally the writer's last one: deref runs the destructor and frees the StaticPipeWriter while &mut self is still a live argument. Every buffer/Blob stdin that drains goes through this (on Linux once memfd is not used, e.g. stdin: Bun.file(..) or BUN_FEATURE_FLAG_DISABLE_MEMFD=1; always on macOS; every shell $ command with a buffer redirected to its stdin).

The receiver is not the only reference involved. The writer is a field of the StaticPipeWriter, and every frame between the poll dispatch and the release held it by reference: PosixPipeWriter::on_poll(&mut self) -> on_write(&mut self) -> PosixBufferedWriter::_on_write(&mut self) -> the vtable shim, which with borrow = mut formed &mut *this over the whole parent. A reference argument is protected for the duration of its call, and both aliasing models reject freeing the allocation it points into while it is live, whether or not anything touches it afterwards. So converting on_write alone would not have been enough; the chain had to go. A reduction of the chain (writer embedded in a refcounted parent, the poll entry holding &mut writer, the parent's report releasing the last ref) fails under Miri with Tree Borrows, the model bun run rust:miri uses, at the deallocation:

error: Undefined Behavior: deallocation through <1716> at alloc239[0x0] is forbidden
  = help: this deallocation (acting as a foreign write access) would cause the protected tag <1707> ... to become Disabled
help: the protected tag <1707> was created here, in the initial state Reserved
   |     fn on_poll_before(&mut self) {

and under Stacked Borrows at the Box::from_raw (not granting access to tag <398> because that would remove [Unique for <1748>] which is strongly protected, <1748> being the same &mut self). The same program with the entry taking *mut Writer passes under both.

The same chain sits under two other final releases today: FileSink::on_write / on_close drop the sink's refs from the poll path through the streaming writer's &mut frames, and SecurityScanSubprocess releases the start() ref right after start(), so for it the owner's release inside writer.close() was the last one, freeing the writer underneath on_write(&mut self), close(&mut self) and close_impl(&mut self).

No crash is known from this; the destructor is the last thing any of these frames do. It is the contract that is wrong, and it is the same contract #36571 moved PipeReader's read/on_poll/done entries onto.

Reduction run under Miri 
struct Writer { parent: *mut Parent }
struct Parent { refs: u32, writer: Writer }

impl Parent {
    // `StaticPipeWriter::on_write`: may free the parent, and so the writer.
    unsafe fn on_write(this: *mut Parent) {
        unsafe {
            (*this).refs -= 1;
            if (*this).refs == 0 { drop(Box::from_raw(this)); }
        }
    }
}

impl Writer {
    // main
    fn on_poll_before(&mut self) {
        let parent = self.parent;
        unsafe { Parent::on_write(parent) }
    }
    // this PR
    unsafe fn on_poll_after(this: *mut Writer) {
        let parent = unsafe { (*this).parent };
        unsafe { Parent::on_write(parent) }
    }
}

fn main() {
    let parent = Box::into_raw(Box::new(Parent { refs: 1, writer: Writer { parent: std::ptr::null_mut() } }));
    let writer: *mut Writer = unsafe { &raw mut (*parent).writer };
    unsafe { (*writer).parent = parent };
    if std::env::args().nth(1).as_deref() == Some("after") {
        unsafe { Writer::on_poll_after(writer) }
    } else {
        unsafe { (*writer).on_poll_before() }
    }
}

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

Fix

Each frame now holds the writer as a raw pointer, does its &mut work through a call-scoped reborrow, and makes the report (or release) with nothing borrowed:

  • PosixPipeWriter (src/io/PipeWriter.rs): on_poll(this: *mut Self, ..) is the poll entry; its &mut half is drain_on_poll, and every arm's final dispatch is the last thing done with this. The on_write, on_error and register_poll hooks take *mut Self too. In both writers _on_write and _on_error are raw, and the on_close they end with (the error path's close, the buffered writer's deferred EOF close, the streaming writer's failed re-arm) is delivered by a raw close_raw, which closes the handle through a statement-scoped borrow and then dispatches on_close with nothing borrowed. close_raw reports exactly when the old code did (a pending unreported close, or a handle that was still open), so close() and the streaming register_poll() are now thin wrappers over the raw entries for the parent's own call paths, which hold the parent alive. The R-2 black_box laundering in the buffered _on_write and both POSIX LaunderedSelf impls go away (the write-path register_poll wrapper still launders, since its receiver is a real &mut self). The parent traits' on_write / on_error / on_close docs state the liveness contract: the parent may free itself from a Drained report, from the on_close after an EndOfFile one, or from the on_close after an error; it has to survive a Pending report and the on_error report itself. One place is deliberately left as it was: drain_buffered_data still reports an error that follows a partial drain from under its own &mut self and is then followed by an on_write for the drained bytes (which is where StaticPipeWriter releases its start() ref on that path). Making that sound for a parent that frees itself from the error's on_close means changing what the partial case reports, i.e. the error-protocol rework that #34697 is about, not the dispatch; the comment there says so.
  • impl_buffered_writer_parent! gains the ptr dispatch mode the streaming macro already had. The mut mode is removed from both macros: this was its only user, and besides the protector it asserted unique access to the writer's own bytes from underneath the writer's frames. The remaining shared users (shell IOWriter, WindowsNamedPipe) keep their writer in an UnsafeCell/JsCell, which the module comment now spells out. src/CLAUDE.md's description of the modes is updated.
  • StaticPipeWriter: borrow = ptr; on_write / on_error / on_close take this: *mut Self and access fields one statement at a time. on_write now says explicitly what was implicit: the writer has to be held across writer.close(). It claims start()'s ref for that when it is still outstanding, takes one of its own otherwise, and releases through this as its last action. The "otherwise" covers the security scanner, which releases start()'s ref right after start(), so before this its free happened inside close(); on its error path (every write error, e.g. EPIPE) the free still happens from on_close, which is what the raw close_raw dispatch is for. The Windows-only release in on_close goes through this as well. start()'s two failure-path releases are unchanged: the caller holds the creation ref across start(), so they are never final (the ref is taken before writer.start() on purpose, a synchronous close on Windows relies on it).
  • EndOfFile is handled differently from before, on purpose. The buffered writer reports EOF after closing the fd and delivers on_close itself once the report returns, so on_write no longer closes the writer there; it releases start()'s ref (not final: the owner's ref is dropped by the on_close that follows) and returns. Before, it closed the writer itself and deliberately kept start()'s ref, which leaked the writer on EOF, and for the scanner the owner's release inside that close freed the writer before the buffered writer's trailing close() read it. The Windows writer never reports EOF, so this is POSIX-only behaviour; EOF itself needs write(2) to return 0 on a pipe, which is why it is reasoned about here rather than tested.
  • shell::io_writer::on_poll takes *mut Poll: it holds an Arc keepalive precisely because the child callbacks may drop the last external ref, and when they do, dropping the keepalive on the way out frees the IOWriter while a &mut Poll argument into it would still be live. dispatch.rs passes the owner raw to both entries.
  • Subprocess::on_close_io and ShellSubprocess::on_close_io no longer borrow the whole StaticPipeWriter (buffer_writer_mut / buffer_mut) to detach its source: they are only ever reached from StaticPipeWriter::on_close, which has already detached it (and Drop detaches again), and they run underneath the writer's own close(), whose &mut to the embedded writer such a borrow would overlap. They just drop the ref now; the two accessors' docs and the on_close contracts say why. The scanner already used a field projection.

The zero-drain error path (on_poll's Err arm, e.g. EPIPE with nothing written in that round) still leaks start()'s ref on POSIX; that is pre-existing, independent of this change (this PR changes how the error path is dispatched, not what it releases) and tracked separately. Once this and #37703 both land, its self-receiver-release allowlist entry for static_pipe_writer.rs drops from 4 to the two start() sites.

Tests

test/internal/source-lints/writer-parent-mut-borrow.test.ts bans borrow = mut at every impl_*_writer_parent! invocation and bans a mut dispatch arm in the macro definitions, checks its scanner against the old StaticPipeWriter invocation, the current FileSink one, a body with braces in an accessor and the macros' own @emit recursion, and requires that it actually found invocations. Against main it reports

src/io/PipeWriter.rs:2603: macro offers a `mut` dispatch mode
src/io/PipeWriter.rs:2741: macro offers a `mut` dispatch mode
src/spawn/static_pipe_writer.rs:69: borrow = mut dispatches through &mut Parent

and passes with this branch. The rest of test/internal/source-lints/ (19 files) passes.

test/cli/install/bun-install-security-provider.test.ts gets a test for the error path: with the same >1MB payload as the existing large-payload test, a bunfig preload makes the scanner process exit before scanner-entry reads its input, so bun's next write fails with EPIPE after one Pending round. On the debug build that is deterministically onWrite(219264 pending) -> onError(EPIPE) -> onClose() -> deref 1 -> 0 (3/3 runs), i.e. the scanner freeing its writer from the error's on_close, now dispatched by close_raw; the install reports Security scanner exited with code 42 without sending data. It passes before and after (nothing observable changes), so it is coverage for the converted path under ASAN rather than the gate test.

Verification

With BUN_DEBUG_StaticPipeWriter=1 BUN_DEBUG_ref_count=1 on the debug (ASAN) build, the three owners now end the same way: onWrite(drained) -> onClose() -> owner's deref 2 -> 1 -> trailing deref 1 -> 0 for Bun.spawn (BUN_FEATURE_FLAG_DISABLE_MEMFD=1, 1 MiB buffer into cat) and for the shell (cat with the same buffer redirected to stdin); for bun pm scan it is ref 1 -> 2 (the ref taken in on_write) -> onClose() -> deref 2 -> 1 -> deref 1 -> 0. A second reduction, one level deeper (the parent's report calls the embedded writer's close(&mut self), whose close_impl(&mut self) writes the handle and then delivers on_close, from which the owner forms a &mut over the whole parent the way on_close_io did), fails under Tree Borrows at that reborrow (reborrow through <tag> ... is forbidden ... would cause the protected tag ... (currently Unique) to become Disabled) and under Stacked Borrows (would remove [Unique for <tag>] which is strongly protected); with the owner touching its field by projection instead, as the owners now do, it passes under both.

On the debug (ASAN) build: test/js/bun/spawn/{spawn,spawnSync,spawn-empty-arrayBufferOrBlob,spawn-stdin-destroy,spawn-pipe-start-error,spawn-many-teardown,spawn-stdin-pipe-fd-leak,spawn-streaming-stdin}.test.ts, test/js/bun/memfd-disabled.test.ts, test/js/bun/shell/{bunshell,shell-blocking-pipe,epipe,shell-write-fault}.test.ts (bunshell: 418 pass), test/js/bun/util/filesink.test.ts, test/js/bun/terminal/terminal.test.ts and test/cli/install/bun-install-security-provider.test.ts all pass. cargo check --target x86_64-pc-windows-msvc for bun_io, bun_spawn, bun_install and bun_runtime passes (the Windows vtable impls and the cfg(windows) branch in on_close are exercised by it); cargo clippy on bun_io, bun_spawn and bun_runtime and cargo fmt --check are clean.