spawn: release the buffer-stdin writer's start() ref when its write fails on POSIX

Merged
RO
3mo ago
+160-55

Problem

  • On POSIX, a Buffer or Blob passed as a child's stdin leaks the native writer that pumps it into the child when the write fails before the buffer drains, typically EPIPE because the child closed its stdin without reading it.
  • Reached by the shell's cmd < ${buffer} redirect on every POSIX configuration, and by Bun.spawn always on macOS but on Linux only when memfd is unavailable.
  • Under LeakSanitizer the shell's stranded writers appear as Direct leak of 1024 byte(s) in 2 object(s) from StaticPipeWriter<ShellSubprocess>::create; with refcount logging the count stays at 1 forever after onError(err=EPIPE: Broken pipe (send())) and onClose().
  • Cause: the writer holds a ref on itself while a write is in flight, and only a drained buffer or the owner closing it at exit released that ref. After a failed write the writer closes itself and the owner empties its slot, so nothing that could release the ref can still reach the writer.

Fix

  • The writer's close callback releases the in-flight ref on every platform, not only Windows. Sound because close is the last callback a parent receives for an error, and every release site claims the same token first, so exactly one of them (drain completion, close after a failure, or the owner) releases.
  • The POSIX drain loop now only writes and cannot invoke callbacks itself; an error is always returned and reported once by the caller. This deletes the arm that #35297 cited when it fixed the same leak on Windows only; the arm was unreachable, so no behaviour changes here.
  • Two rarer error paths get the same report-then-close shape: a failed poll re-registration used to report without closing, and a zero-byte write closed without releasing. Both are reasoned about, not tested. Out of scope, pre-existing: on Windows a write failing synchronously inside start() still strands the ref; tracked separately.
  • Verification: a new ASAN-lane test runs a fixture under detect_leaks=1 that drives both owners through failing and draining writes. It fails 4 of 4 runs without the fix with the report above and passes 5 of 5 with it. LSan only reports the shell-owned writers; the Bun.spawn cases exercise the same teardown without being what fails.

Background

  • The static pipe writer is the native object that writes one fixed in-memory buffer (a Buffer or Blob stdin) into a child's stdin pipe. It is refcounted: one ref lives in the owning process's stdin slot, a second is taken when the write starts so the object outlives the write.
  • It has two owners: a Bun.spawn subprocess and a shell subprocess for the < ${buffer} redirect. On Linux Bun.spawn normally hands the child a memfd and never creates the writer, which is why the test disables memfd; the shell redirect always uses it.
  • The buffered pipe writer underneath reports to its parent through three callbacks: bytes drained, error, closed. Parents are written assuming closed is the last one they receive, so freeing the object there is safe only if nothing underneath touches it afterwards.
  • The started flag is the token for the in-flight ref: whichever site swaps it to false does the release, so three possible release sites cannot release twice.
  • EPIPE is what writing to a pipe returns once the reader has closed its end; it is how a child closing its stdin reaches the parent while the buffer is still being written.

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts

Original description

On POSIX, a Buffer/Blob stdin whose write fails before it has drained (typically EPIPE, because the child closed its stdin without reading it) leaks the native StaticPipeWriter that pumps the buffer into the child. Two owners share that writer: the shell's cmd < ${buffer} redirect, which uses it on every POSIX configuration, and Bun.spawn, which on Linux only uses it when memfd is unavailable (BUN_FEATURE_FLAG_DISABLE_MEMFD=1, or no memfd_create; otherwise the child gets a memfd) and on macOS always.

Repro

// BUN_FEATURE_FLAG_DISABLE_MEMFD=1 BUN_DEBUG_StaticPipeWriter=1 BUN_DEBUG_ref_count=1 bun-debug run repro.ts
const big = Buffer.alloc(1 << 20, 0x61);
const proc = Bun.spawn({ cmd: ["sh", "-c", "exec 0<&-; sleep 0.3"], stdin: big, stdout: "ignore" });
console.log("exit", await proc.exited);

Before, for the writer:

StaticPipeWriter(0x..) start()
0x..   ref 1 -> 2                      start()
StaticPipeWriter(0x..) onError(err=EPIPE: Broken pipe (send()))
StaticPipeWriter(0x..) onClose()
0x.. deref 2 -> 1                      owner drops create()'s ref in on_close_io

and the count stays at 1 forever. After, the same trace continues with deref 1 -> 0. $\cmd < ${big}`` with a command that exits without reading stdin shows the same before/after (no memfd flag involved), and under LeakSanitizer the shell's stranded writers come out as

Direct leak of 1024 byte(s) in 2 object(s) allocated from:
    ...
    #12 <StaticPipeWriter<ShellSubprocess>>::create src/spawn/static_pipe_writer.rs:110
    #13 <shell::subproc::Writable>::init src/runtime/shell/subproc.rs:1149

Cause

start() takes a ref on the writer and sets started; that ref was released from on_write when the buffer drained, or by the owner (Subprocess::on_process_exit / close_io) if it closed the writer while the slot still held it. A failed write takes neither route: PosixBufferedWriter::_on_error runs on_error and then close(), on_close tells the owner, the owner empties its slot and drops create()'s ref, and nothing is left that can find the writer to release start()'s ref.

#35297 fixed exactly this on Windows by releasing in on_close, and left POSIX alone because of the shape of PosixPipeWriter::drain_buffered_data: it had an arm that, for an error after a partial drain, called on_error inline (closing the writer) and returned Wrote(n), after which on_poll delivered on_write(n, Drained) to the same object, so a release in on_close looked unsafe. That arm was in fact unreachable (try_write reports a short write as Pending, never as Wrote, so the drain loop never gets to an error with bytes already drained), but as written it was the documented reason the POSIX release site did not exist.

Fix

  • drain_buffered_data (src/io/PipeWriter.rs) loses that arm and takes &self, so it structurally cannot dispatch callbacks: an error is always returned as Err and reported once by the caller, and on_close is the last callback a parent sees. That is the contract the parents are written against (FileSink::on_close releases its keep-alive as "the last thing", Terminal::on_writer_close derefs the terminal, FileSink::on_auto_flush documents flush() returning Err as the error contract); this makes it explicit rather than a property of which arms happen to be reachable. No behaviour changes here.
  • StaticPipeWriter::on_close (src/spawn/static_pipe_writer.rs) claims and releases start()'s ref on every platform, not just Windows. This is the behavioural fix. The release is the last access: the frames underneath it (close_impl -> close -> _on_error -> on_poll) do nothing with the writer after the callback, which is the same shape SecurityScanSubprocess (whose owner ref is the last one) already relies on in on_close_io today. on_write keeps taking the token before it closes and the owners keep taking it before they close, so exactly one site releases the ref.
  • PosixBufferedWriter::register_poll reports a failed re-registration through _on_error (report, then close()), like every other error report and like the streaming writer's register_poll already did, instead of reporting and leaving the writer open. Raised in review: that (ENOMEM-class) failure was the one report not followed by the on_close that now releases the ref; Bun.spawn would still have picked the writer up at exit, the shell had nothing to pick it up with. Reasoned about rather than tested, since it needs the registration syscall itself to fail.
  • The EndOfFile arm of on_write releases the ref as well (non-final, since the owner's ref is dropped by the on_close the buffered writer delivers right after) instead of closing the writer itself and leaving the ref outstanding. That arm needs write(2) to return 0 on a pipe, so it is reasoned about rather than tested; it is the same change #37755 makes to this arm.

Known gap, pre-existing and not touched here: on Windows a uv_write that fails synchronously inside start() closes the writer (on_close runs, the owner drops its ref) before start() sets started, which strands the ref the same way. Fixing it needs SecurityScanSubprocess's post-start() release to become token-aware as well, so it is tracked separately.

Related: #34697 proposes the same drain_buffered_data change as one item of a broader hardening pass, but is 600+ commits behind with conflicts in three files. #37755 moves these callbacks onto raw this pointers and leaves the error chain (and this leak) alone; the two conflict textually in on_close / on_poll but compose, whichever lands second needs to keep the unconditional release in on_close and the Err return in drain_buffered_data.

Test

test/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts runs a fixture on the ASAN lane with detect_leaks=1 (the setup from shell-worker-terminate-leak.test.ts). The fixture drives both owners through both endings: children that close their stdin, report it and wait to be killed (so the write fails while the child is alive and the exit path cannot be what cleans up), and children that drain it; memfd is disabled so that the Bun.spawn scenarios reach the writer at all. Unfixed it fails every time (4 of 4) with the report quoted above: the two shell-owned writers. The two stranded Bun.spawn-owned writers from the same run have their refcount stuck at 1 just the same (checked with BUN_DEBUG_ref_count, and their Subprocess structs are freed before exit), but LSan still finds their address somewhere and does not report them, so those scenarios only exercise the same teardown under ASAN. Fixed, the fixture is clean, 5 of 5 runs, about 0.7s each.

An earlier revision used a live-writer counter in bun:internal-for-testing; it was removed per review in favour of LSan.

Verification

Debug (ASAN) build: the file above, plus spawn.test.ts, spawnSync.test.ts, spawn-empty-arrayBufferOrBlob, spawn-streaming-stdin, spawn-stdin-readable-stream, spawn-stdin-destroy, spawn-pipe-start-error, spawn-many-teardown, memfd-disabled, bunshell.test.ts (418 pass), bunshell-instance, epipe, shell-blocking-pipe, shell-write-fault, file-io, pipeline_stack, filesink.test.ts, terminal.test.ts, terminal-spawn, child_process.test.ts and bun-security-scanner-workspaces all pass. cargo check of bun_io and bun_spawn for x86_64-pc-windows-msvc and aarch64-apple-darwin, cargo clippy on both, and cargo fmt --check are clean. CI on the two previous revisions (the first of which ran its tests on every lane, Windows included) was green apart from flaky tests that passed on retry.