io: POSIX pipe writers hold a ref on their parent from on_error until close() returns

Closed
RO
19d ago
+176-20

Problem

  • A script pipes a type: "direct" ReadableStream into stdout with Bun.write and awaits controller.flush(). The reader exits. An ASAN build reports heap-use-after-free in PosixStreamingWriter<FileSink>::close (src/io/PipeWriter.rs:1076), called from _on_error.
  • _on_error calls Parent::on_error, then self.close(). FileSink::on_error rejects the flush promise, and the rejection handlers release the last refs on the sink. The writer is a field of the sink.

Fix

  • Both POSIX pipe writers hold a ref on the parent across on_error and close(): in _on_error, and in the failed-registration arm of PosixStreamingWriter::register_poll. Nothing uses the writer after close().
  • ParentKeepAlive is the RAII guard. The parent macros stamp its trait, PosixWriterParent, from the ref_ and deref that each parent already passes for Windows. The arm of register_poll is a #[cold] function, so register_poll still inlines into write.
  • Verified: test/js/bun/util/filesink.test.ts (main's ASAN build fails the new test). Also the terminal, shell and spawn suites.
  • Self-reviewed: 3 concerns raised, 3 addressed (see Notes).

Background

  • A pipe writer drains bytes to an fd and reports to its refcounted parent (FileSink, Terminal, the shell IOWriter, StaticPipeWriter) through on_write, on_error and on_close.
  • Bun.write(dest, stream) pumps into a FileSink with no JS wrapper. Two refs hold it: the pump's, and a keep-alive ref that on_close releases.
  • Considered a guard inside FileSink::on_error: it ends before the writer calls close(). The writer's guard covers every parent.

Downsides

  • A failed write costs one more increment and decrement of the parent's refcount.
  • A write that succeeds costs nothing new: 346 instructions in FileSink::write for a 64-byte write, on main and here. Text grows by 768 bytes.
  • Not changed: the Windows writers, and uses of a writer after on_write (see Notes).
Notes

This is a rebase of the July version of this PR. What changed in it:

  • Gone, because main has them now: the FIFO redirect change in shell/Builtin.rs and its test (#34696), drain_buffered_data that returns the error and does not call on_error (#37774), the release of the start() ref of StaticPipeWriter after a failed write (#37774 does it in on_close), and PosixBufferedWriter::register_poll, which now reports a failed registration through _on_error.
  • Gone, because the code is gone: the macOS libinfo hunk in dns.rs.
  • Dropped: the STATIC_PIPE_WRITER dispatch shim (it existed for the on_error release that #37774 replaced, and #37755 reworks that chain), the keepalive in IOWriter::on_write_pollable (the poll dispatch already holds one, and on Windows the in-flight write holds one), and the comment edits in shell/IOWriter.rs and shell/subproc.rs.
  • Changed: the guard is a named Drop type, not a scopeguard closure. ref_ and deref are on one supertrait, not on each POSIX parent trait.

Repro (Linux x64, debug ASAN build):

cat > sink.mjs <<'JS'
await Bun.write(Bun.stdout, new Response(new ReadableStream({ type: "direct",
  async pull(c) { c.write(new Uint8Array(1 << 20)); await c.flush(); c.close(); } })));
JS
bun sink.mjs | (sleep 0.1; head -c 262144 > /dev/null)

main at 11c41c645d: 3 of 3 runs report the use-after-free. This branch: 0 of 3, exit status 1 with one EPIPE: broken pipe, write, the same as a release build.

heap-use-after-free, READ of size 1, 121 bytes inside of a 344-byte region
 #0 PosixStreamingWriter<FileSink>::close       src/io/PipeWriter.rs:1076
 #1 PosixStreamingWriter<FileSink>::_on_error   src/io/PipeWriter.rs:790
 #3 PosixPipeWriter::on_poll                    src/io/PipeWriter.rs:202
freed by
#17 FileSink::run_pending                       src/runtime/webcore/FileSink.rs:362
#18 FileSink::on_error                          src/runtime/webcore/FileSink.rs:483
#20 PosixStreamingWriter<FileSink>::_on_error   src/io/PipeWriter.rs:789

How the last ref goes away inside on_error. run_pending rejects the flush promise and runs the microtasks. pull() rejects, so the promise of the pump rejects and on_reject_stream runs. It adopts the ref of the pump and calls handle_reject_stream, which calls writer.close(). _on_error already closed the fd without a report, so this close() delivers on_close, and on_close releases the keep-alive ref. on_reject_stream then drops the ref of the pump, and the guard in run_pending drops the last one.

With the change the close() at the end of _on_error finds the handle closed and reports nothing. The sink gets one on_error and one on_close. In the release build the flag closed_without_reporting is read from memory again after the on_error call (checked in the disassembly of __bun_run_file_poll), so a value from before the call is not used. The guard is three instructions there: incl, then decl and jne in front of the cold call that frees the sink.

Cost of a write that succeeds. register_poll runs on every buffered write. With the guard inline in its error arm, the compiler stopped inlining register_poll into write. The arm is out of line now. I counted user instructions from the entry of FileSink::write to its return with gdb (stepi), over 128 consecutive calls, in release builds on the same base (5a183c1ebc). The script writes 64 bytes to proc.stdin in a loop and calls flush() every 64 writes.

buildbuffered writewrite that reaches 4096 bytesaverage
main346386346.6
guard inline in the arm (f29b67c318)363384363.3
#[cold] arm (this head)346384346.6

size of the stripped binary: text 80,677,348 on main and 80,678,116 here. Both files are 80,844,360 bytes. perf and valgrind are not available in my container, so I have no count for a whole process.

The test. A write error reaches the sink from two places. One is the poll callback (this bug). The other is the flush that the auto-flusher of the sink makes after each microtask drain. That one has its own guard and does not reach _on_error. So the read end has to close while the child waits in its poll. The test reads 4 x 64 KiB from a FIFO, waits 10 ms, and then closes. Each refill shows that the child ran, and the wait lets it finish the turn that made the last refill. On the debug ASAN build of main the test fails 5 of 5 times alone and 2 of 2 times inside the whole file. The same sequence in a script, 6 at a time, reports the use-after-free 18 of 18 times. A fixed build gives the same output on both paths, so the wait cannot make the test fail there.

An earlier version closed in the same step as its last read. That finds the bug only when the close lands before the next write of the child. A fast build often writes first, and then the flush at the end of that turn reports the error.

Not changed: Windows. The Windows writers take a ref on the parent for each write in flight and release it after the completion callback. That covers this order there. The three arms of WindowsStreamingWriter::process_send for a write that fails at once call on_error and then close() with no ref of their own. I found no way to reach them with a promise pending, and I cannot run Windows.

Not changed: the free under &mut self. The release of the guard can free the parent while _on_error(&mut self) is on the stack. Main already frees at this position (StaticPipeWriter::on_close, FileSink::on_close), and #37755 moves that chain to raw pointers.

Not changed: uses of the writer after on_write. There are nine on POSIX: six in the write* functions of PosixStreamingWriter (on_write, then register_poll), the Pending arm of on_poll for each of the two writers, and PosixBufferedWriter::_on_write (on_write, then close() at end of file). I found no input that frees the parent inside them. Only facts about each parent prevent it, and no written rule does. A guard there adds a ref and a deref to every write. #37755 reworks that chain and is the place for the rule.

Self-review. Three concerns survived, and each is handled. The SAFETY note in StaticPipeWriter::on_close said that its deref frees the writer on POSIX. With the new ref it does not, so the note is updated, and so is the comment in FileSink::on_error. Both POSIX parent traits now say what on_error may release. The unguarded sites are listed above. #37755 rewrites the same three functions: the one that lands second has to carry the ref.

Checks. cargo fmt, cargo clippy for bun_io, bun_spawn and bun_runtime, cargo check for x86_64-pc-windows-msvc and aarch64-apple-darwin. On the debug ASAN build: filesink.test.ts (78 pass), terminal.test.ts (99), console-write.test.ts (5), shell/file-io.test.ts (27), shell-blocking-pipe.test.ts (2), shell-write-fault.test.ts (1), spawn-stdio-syscall-error.test.ts (17), spawn-stdin-readable-stream.test.ts (37), spawn-stdin-pipe-fd-leak.test.ts (4). spawn.test.ts ran on an overloaded host: 170 pass and 8 timeouts at 5 s here, 172 pass and 6 timeouts in the same groups on the build of main.


no test proof · iteration 10 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts