Skip to content
Merged
6 changes: 5 additions & 1 deletion src/runtime/webcore/FileReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ impl FileReader {
{
use bun_io::pipe_reader::PosixFlags;
if !self.started.get()
&& !self.waiting_for_on_reader_done.get()
&& self.reader().flags.contains(PosixFlags::POLLABLE)
&& !self.reader().is_done()
{
Expand All @@ -454,7 +455,10 @@ impl FileReader {
// Non-lazy fromPipe path (Bun.spawn stdout/stderr): hold a
// ref across the pending uv_read_start so the source is not
// finalized while IOCP has a read queued on it.
if !self.started.get() && self.reader().source.is_some() && !self.reader().is_done()
if !self.started.get()
&& !self.waiting_for_on_reader_done.get()
&& self.reader().source.is_some()
&& !self.reader().is_done()
{
self.waiting_for_on_reader_done.set(true);
// SAFETY: see `parent()`.
Expand Down
15 changes: 14 additions & 1 deletion src/runtime/webcore/ReadableStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,20 @@ impl ReadableStream {
.reader()
.from(buffered_reader, ctx_ptr.cast::<c_void>());

source.to_readable_stream(global_this)
let stream = source.to_readable_stream(global_this)?;

// The transferred reader already has a live poll registered with the
// event loop whose owner now points into this allocation. Take the
// across-read ref (and root the wrapper) immediately so a GC before
// JS first pulls cannot sweep the wrapper and free this box while the
// poll is still armed. `on_start` checks `waiting_for_on_reader_done`
// and will not take a second ref.
if source.context.reader().has_pending_activity() {
source.context.waiting_for_on_reader_done.set(true);
source.increment_count();
}

Ok(stream)
}

pub fn empty(global_this: &JSGlobalObject) -> JsResult<JSValue> {
Expand Down
119 changes: 119 additions & 0 deletions test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import { join } from "node:path";

// Accessing `proc.stdout` on a piped subprocess moves the already-registered
// pipe poll from the subprocess PipeReader into a freshly allocated
// NewSource<FileReader>. The across-read ref (which upgrades the wrapper's
// JsRef to Strong and pins the native box) used to be taken only in
// FileReader::on_start, the first time JS actually pulls from the stream.
// Between from_pipe and that first pull the poll's owner points into a box
// whose only ref is the JS wrapper's own Weak back-reference, so if the
// subprocess and its cached stdout become unreachable before anyone pulls,
// GC can sweep the wrapper and free the box while the poll is still armed.
// The next readability/EOF event then dispatches into freed memory
// (FileReader::on_reader_done reached from PosixBufferedReader::read_socket).
//
// This test asserts the lifetime invariant directly rather than racing for
// the crash: while a subprocess pipe's write end is still held open (by a
// detached grandchild that inherits it), the FileInternalReadableStreamSource
// wrapper must survive GC even though nothing in JS references it. Once the
// grandchild exits and the pipe reaches EOF, on_reader_done releases the ref
// and the wrapper becomes collectable (no leak).
//
// Windows uses libuv for pipe I/O; the from_pipe path under test is POSIX.
test.skipIf(isWindows)(
"subprocess stdout FileReader is pinned while its pipe poll is live, then collectable after EOF",
async () => {
using dir = tempDir("fr-gc-uaf", {});
const flag = join(String(dir), "go");

const script = /* js */ `
const { heapStats } = require("bun:jsc");
const { writeFileSync } = require("node:fs");
const ITERS = 4;
const flag = ${JSON.stringify(flag)};

// The direct child spawns a detached shell that inherits stdout and
// waits for a flag file, keeping the pipe's write end open past the
// child's exit so the FileReader's poll is still armed while we GC.
const childScript =
"const { spawn } = require('child_process');" +
"spawn('sh', ['-c', 'while [ ! -e " + JSON.stringify(flag) + " ]; do sleep 0.02; done; echo x']," +
" { stdio: ['ignore', 'inherit', 'ignore'], detached: true }).unref();";

Check warning on line 43 in test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts

View check run for this annotation

Claude / Claude Code Review

Detached sh grandchildren leak forever if inner script exits before writing flag

The detached `sh` grandchildren busy-wait on the flag file with no upper bound, and the flag is only written at line 75 with no `try`/`finally` — if the inner script crashes, hangs, or is killed by the 60s test timeout before reaching `writeFileSync(flag, '')`, all 5 detached shells spin forever (and `using dir` deletes the temp dir so the flag path can never appear). Bound the loop, e.g. `i=0; while [ ! -e FLAG ] && [ $i -lt 500 ]; do sleep 0.02; i=$((i+1)); done; echo x`, or wrap the inner scr
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment thread
robobun marked this conversation as resolved.
Resolved

const count = () =>
heapStats().objectTypeCounts.FileInternalReadableStreamSource ?? 0;

async function once() {
const proc = Bun.spawn({
cmd: [process.execPath, "-e", childScript],
env: process.env,
stdout: "pipe",
stderr: "ignore",
stdin: "ignore",
});
// Materialize the ReadableStream so the live poll is re-parented
// into a NewSource<FileReader>, but never pull from it.
void proc.stdout;
await proc.exited;
}

// Warm up once so per-class lazy structure allocation is absorbed
// into the baseline and not counted against ITERS.
await once();
for (let i = 0; i < 10; i++) { Bun.gc(true); await Bun.sleep(1); }
const base = count();

await Promise.all(Array.from({ length: ITERS }, once));

// Direct children have exited; grandchildren still hold the write end.
for (let i = 0; i < 20; i++) { Bun.gc(true); await Bun.sleep(1); }
const duringLivePipe = count() - base;

// Release the grandchildren (including the warmup one).
writeFileSync(flag, "");

let afterEof = -1;
for (let i = 0; i < 100; i++) {
Bun.gc(true);
await Bun.sleep(10);
afterEof = count() - base;
if (afterEof <= 0) break;
}

console.log(JSON.stringify({ iters: ITERS, duringLivePipe, afterEof }));
`;

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,
]);

const stderrFiltered = stderr
.split("\n")
.filter(l => l && !l.startsWith("WARNING: ASAN interferes"))
.join("\n");
expect(stderrFiltered).toBe("");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

const { iters, duringLivePipe, afterEof } = JSON.parse(stdout.trim());
// Invariant: every FileReader whose pipe poll is still armed must be
// pinned by its own Strong ref. Before the fix they were swept here
// (duringLivePipe ~ 0), which is exactly the UAF precondition.
expect(duringLivePipe).toBeGreaterThanOrEqual(iters);
// Once the pipe reaches EOF the ref is released and the wrappers become
// collectable. One may survive via a conservatively-rooted final
// Subprocess (same caveat as spawn-ipc-gc.test.ts).
expect(afterEof).toBeLessThanOrEqual(1);
expect(exitCode).toBe(0);
},
60_000,
);
Loading