Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/runtime/webcore/FileReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
robobun marked this conversation as resolved.
Outdated
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
Expand Down
90 changes: 90 additions & 0 deletions test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,3 +392,93 @@ 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<FileReader> leak in release and a `done && waiting` debug_assert
// at finalize_detach in assertion builds).
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
test.skipIf(isWindows)(
"cancelling a subprocess stdout FileReader before any data releases the io-ref",
async () => {
Comment thread
robobun marked this conversation as resolved.
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 {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
30_000,
);
Loading