io: POSIX pipe writers hold a ref on their parent from on_error until close() returns
Problem
- A script pipes a
type: "direct"ReadableStream into stdout withBun.writeand awaitscontroller.flush(). The reader exits. An ASAN build reportsheap-use-after-freeinPosixStreamingWriter<FileSink>::close(src/io/PipeWriter.rs:1076), called from_on_error. _on_errorcallsParent::on_error, thenself.close().FileSink::on_errorrejects 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_errorandclose(): in_on_error, and in the failed-registration arm ofPosixStreamingWriter::register_poll. Nothing uses the writer afterclose(). ParentKeepAliveis the RAII guard. The parent macros stamp its trait,PosixWriterParent, from theref_andderefthat each parent already passes for Windows. The arm ofregister_pollis a#[cold]function, soregister_pollstill inlines intowrite.- 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 shellIOWriter,StaticPipeWriter) throughon_write,on_errorandon_close. Bun.write(dest, stream)pumps into aFileSinkwith no JS wrapper. Two refs hold it: the pump's, and a keep-alive ref thaton_closereleases.- Considered a guard inside
FileSink::on_error: it ends before the writer callsclose(). 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::writefor 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.rsand its test (#34696),drain_buffered_datathat returns the error and does not callon_error(#37774), the release of thestart()ref ofStaticPipeWriterafter a failed write (#37774 does it inon_close), andPosixBufferedWriter::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_WRITERdispatch shim (it existed for theon_errorrelease that #37774 replaced, and #37755 reworks that chain), the keepalive inIOWriter::on_write_pollable(the poll dispatch already holds one, and on Windows the in-flight write holds one), and the comment edits inshell/IOWriter.rsandshell/subproc.rs. - Changed: the guard is a named
Droptype, not ascopeguardclosure.ref_andderefare 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.
| build | buffered write | write that reaches 4096 bytes | average |
|---|---|---|---|
| main | 346 | 386 | 346.6 |
| guard inline in the arm (f29b67c318) | 363 | 384 | 363.3 |
#[cold] arm (this head) | 346 | 384 | 346.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