Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions src/runtime/webcore/FileReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,15 @@ impl FileReader {
if !self.reader().is_done() {
self.reader().close();
}
// `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`.
Comment thread
robobun marked this conversation as resolved.
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