From 068086a1c77fc44f27f2df9391387e58981f453f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:22:33 +0000 Subject: [PATCH 1/5] FileReader: release the io-ref in on_cancel when close() doesn't reach on_reader_done test-stdin-from-file-spawn.js panicked on linux x64-asan with assertion failed: !(self.done.get() && self.waiting_for_on_reader_done.get()) in FileReader::finalize_detach. on_cancel sets done=true and calls reader().close(); the expectation is that close() synchronously reaches on_reader_done which clears waiting_for_on_reader_done and releases the io-ref taken by from_pipe/on_start. PollOrFd::close_impl skips the callback when the handle's fd is already invalid (and close_handle is a no-op when CLOSE_HANDLE is unset), so on_cancel can return with both flags set and the io-ref stranded. Release it in on_cancel; idempotent with on_reader_done's own clear. --- src/runtime/webcore/FileReader.rs | 14 +++ .../spawn-stdout-filereader-gc-uaf.test.ts | 95 +++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/src/runtime/webcore/FileReader.rs b/src/runtime/webcore/FileReader.rs index b205c96b724c..458d36e5d36e 100644 --- a/src/runtime/webcore/FileReader.rs +++ b/src/runtime/webcore/FileReader.rs @@ -537,6 +537,20 @@ impl FileReader { if !self.reader().is_done() { self.reader().close(); } + // `close()` normally reaches `on_reader_done` (which clears + // `waiting_for_on_reader_done` and releases the io-ref), but + // `PollOrFd::close_impl` skips the callback when the handle's fd is + // already invalid, and `close_handle` is a no-op when `CLOSE_HANDLE` + // is unset. In either case the io-ref taken in `on_start`/`from_pipe` + // is stranded and `finalize_detach` would observe `done && waiting`. + // Release it here; idempotent with `on_reader_done`'s own clear. + if self.waiting_for_on_reader_done.get() { + self.waiting_for_on_reader_done.set(false); + let parent = self.parent(); + // SAFETY: see `parent()`. `waiting` implies `increment_count` ran, + // so `ref_count >= 2` and this cannot free the box. + let _ = unsafe { Source::decrement_count(parent) }; + } } // NOTE: not `impl Drop` — FileReader is embedded as `Source.context` and this is diff --git a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts index f1435ae8f0b4..cad0103a88dd 100644 --- a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts +++ b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts @@ -392,3 +392,98 @@ test.skipIf(isWindows)( }, 30_000, ); + +// FileReader::on_cancel sets `done = true` and calls `reader().close()`. In the +// normal case `close()` reaches `on_reader_done`, which clears +// `waiting_for_on_reader_done` and releases the io-ref taken by `from_pipe`. +// If `close()` does not reach `on_reader_done` (PollOrFd::close_impl skips the +// callback when the handle's fd is already invalid), the io-ref is stranded +// with `done = true` and `finalize_detach` later observes `done && waiting`, +// which is a debug_assert in assertion builds and a refcount leak in release. +// +// This test asserts the post-cancel invariant directly: after cancelling a +// from_pipe FileReader, the Strong pin must be released so the wrapper becomes +// collectable. It cancels before any data arrives (the io-ref is the only +// thing keeping the wrapper protected) so a stranded ref shows up as a +// surviving FileInternalReadableStreamSource after GC. +test.skipIf(isWindows)( + "cancelling a subprocess stdout FileReader before any data releases the io-ref", + async () => { + const script = /* js */ ` + const { heapStats } = require("bun:jsc"); + const count = () => + heapStats().objectTypeCounts.FileInternalReadableStreamSource ?? 0; + const prot = () => + heapStats().protectedObjectTypeCounts.FileInternalReadableStreamSource ?? 0; + + // Warm up so per-class lazy structure allocation is in the baseline. + { + const p = Bun.spawn({ + cmd: ["sleep", "0.3"], + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + lazy: true, + }); + await p.stdout.cancel(); + p.kill(); + await p.exited; + } + for (let i = 0; i < 10; i++) { Bun.gc(true); await Bun.sleep(1); } + const base = count(); + + const ITERS = 6; + async function once() { + // lazy:true mirrors child_process: from_pipe transfers an Fd-backed + // handle (poll registration is deferred), so the only Strong root is + // the io-ref that on_cancel must release. + const p = Bun.spawn({ + cmd: ["sleep", "5"], + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + lazy: true, + }); + // Materialize the FileReader (waiting_for_on_reader_done = true) and + // cancel it before any data can arrive. + await p.stdout.cancel(); + // The Subprocess wrapper caches stdout and has_pending_activity keeps + // it alive while the process runs, so reap it now and drop every JS + // reference; only the io-ref can keep the source alive past here. + p.kill(); + await p.exited; + } + for (let i = 0; i < ITERS; i++) await once(); + + const protectedAfterCancel = prot(); + for (let i = 0; i < 20; i++) { Bun.gc(true); await Bun.sleep(1); } + const aliveAfterCancel = count() - base; + + console.log(JSON.stringify({ + iters: ITERS, base, protectedAfterCancel, aliveAfterCancel, + })); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "--smol", "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + let result: { iters: number; protectedAfterCancel: number; aliveAfterCancel: number }; + try { + result = JSON.parse(stdout.trim()); + } catch { + throw new Error(`fixture did not emit JSON (exit ${exitCode})\nstdout: ${stdout}\nstderr: ${stderr}`); + } + // After cancel, nothing should keep the wrapper Strong-protected. + expect(result.protectedAfterCancel).toBe(0); + // The wrappers must be collectable; one may survive via a conservatively + // rooted stack slot (same caveat as the first test in this file). + expect(result.aliveAfterCancel).toBeLessThanOrEqual(1); + expect(exitCode).toBe(0); + }, + 30_000, +); From 245a87a0146249b7ba223818bf5eb30c17a07122 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:51:10 +0000 Subject: [PATCH 2/5] test: reframe on_cancel io-ref test as a post-cancel invariant check --- .../spawn-stdout-filereader-gc-uaf.test.ts | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts index cad0103a88dd..8d05a7fd6df8 100644 --- a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts +++ b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts @@ -393,19 +393,14 @@ test.skipIf(isWindows)( 30_000, ); -// FileReader::on_cancel sets `done = true` and calls `reader().close()`. In the -// normal case `close()` reaches `on_reader_done`, which clears -// `waiting_for_on_reader_done` and releases the io-ref taken by `from_pipe`. -// If `close()` does not reach `on_reader_done` (PollOrFd::close_impl skips the -// callback when the handle's fd is already invalid), the io-ref is stranded -// with `done = true` and `finalize_detach` later observes `done && waiting`, -// which is a debug_assert in assertion builds and a refcount leak in release. -// -// This test asserts the post-cancel invariant directly: after cancelling a -// from_pipe FileReader, the Strong pin must be released so the wrapper becomes -// collectable. It cancels before any data arrives (the io-ref is the only -// thing keeping the wrapper protected) so a stranded ref shows up as a -// surviving FileInternalReadableStreamSource after GC. +// Post-cancel invariant: after cancelling a from_pipe FileReader, the Strong +// pin (the io-ref taken by `from_pipe`) must be released so the wrapper is +// collectable. This scenario reaches FileReader::on_cancel with +// waiting_for_on_reader_done = true; on current main the release happens via +// close() → on_reader_done. It guards against any future change that makes +// on_cancel return with the io-ref still held (which would be a silent +// NewSource leak in release and a `done && waiting` debug_assert +// at finalize_detach in assertion builds). test.skipIf(isWindows)( "cancelling a subprocess stdout FileReader before any data releases the io-ref", async () => { From 37243292d7b0d74e125238297ea08e4b7901b74f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:52:52 +0000 Subject: [PATCH 3/5] shorten on_cancel io-ref release comment --- src/runtime/webcore/FileReader.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/runtime/webcore/FileReader.rs b/src/runtime/webcore/FileReader.rs index 458d36e5d36e..87f12454ea62 100644 --- a/src/runtime/webcore/FileReader.rs +++ b/src/runtime/webcore/FileReader.rs @@ -537,13 +537,9 @@ impl FileReader { if !self.reader().is_done() { self.reader().close(); } - // `close()` normally reaches `on_reader_done` (which clears - // `waiting_for_on_reader_done` and releases the io-ref), but - // `PollOrFd::close_impl` skips the callback when the handle's fd is - // already invalid, and `close_handle` is a no-op when `CLOSE_HANDLE` - // is unset. In either case the io-ref taken in `on_start`/`from_pipe` - // is stranded and `finalize_detach` would observe `done && waiting`. - // Release it here; idempotent with `on_reader_done`'s own clear. + // `close()` has no-op paths that never dispatch `on_reader_done` + // (`PollOrFd::close_impl` with an already-invalid fd); release the + // io-ref here so `finalize_detach` never sees `done && waiting`. if self.waiting_for_on_reader_done.get() { self.waiting_for_on_reader_done.set(false); let parent = self.parent(); From 5c7f61b18be11a4f70886b9c31a19e2dc7ab5bdd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:28:18 +0000 Subject: [PATCH 4/5] reword on_cancel comment: io-ref ownership, not a close() no-op claim --- src/runtime/webcore/FileReader.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/runtime/webcore/FileReader.rs b/src/runtime/webcore/FileReader.rs index 87f12454ea62..593cda8136ac 100644 --- a/src/runtime/webcore/FileReader.rs +++ b/src/runtime/webcore/FileReader.rs @@ -537,9 +537,8 @@ impl FileReader { if !self.reader().is_done() { self.reader().close(); } - // `close()` has no-op paths that never dispatch `on_reader_done` - // (`PollOrFd::close_impl` with an already-invalid fd); release the - // io-ref here so `finalize_detach` never sees `done && waiting`. + // `done = true` means no more io, so the io-ref is this function's to + // release; `close()` is not contract-bound to dispatch `on_reader_done`. if self.waiting_for_on_reader_done.get() { self.waiting_for_on_reader_done.set(false); let parent = self.parent(); From 8a260d7b8c8b4b979503ee89e7cbbbd2cd0be26a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:34:02 +0000 Subject: [PATCH 5/5] test: drop implementation-history clause from the post-cancel invariant comment --- .../spawn/spawn-stdout-filereader-gc-uaf.test.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts index 8d05a7fd6df8..e812a32f7193 100644 --- a/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts +++ b/test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts @@ -393,14 +393,11 @@ test.skipIf(isWindows)( 30_000, ); -// Post-cancel invariant: after cancelling a from_pipe FileReader, the Strong -// pin (the io-ref taken by `from_pipe`) must be released so the wrapper is -// collectable. This scenario reaches FileReader::on_cancel with -// waiting_for_on_reader_done = true; on current main the release happens via -// close() → on_reader_done. It guards against any future change that makes -// on_cancel return with the io-ref still held (which would be a silent -// NewSource leak in release and a `done && waiting` debug_assert -// at finalize_detach in assertion builds). +// Post-cancel invariant: after cancelling a from_pipe FileReader that never +// received data, the io-ref taken by `from_pipe` must be released so the +// wrapper is collectable. A stranded ref is a silent NewSource +// leak in release and a `done && waiting` debug_assert at finalize_detach in +// assertion builds. test.skipIf(isWindows)( "cancelling a subprocess stdout FileReader before any data releases the io-ref", async () => {