Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
22 changes: 17 additions & 5 deletions src/io/PipeReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1501,9 +1501,13 @@ impl WindowsBufferedReader {
// ALWAYS complete the read first (cleans up fs_t, updates state)
file.complete(was_canceled);

// If detached, file should be closing itself now
if parent_ptr.is_null() {
debug_assert!(file.state == crate::source::FileState::Closing); // complete should have started close
if file.state != crate::source::FileState::Closing {
// detach_borrowed_fd path: no close scheduled, so reclaim here.
// SAFETY: sole &mut to the into_raw'd Box; no fs callback left.
drop(unsafe { bun_core::heap::take(core::ptr::from_mut(file)) });
}
// else: detach() set close_after_operation; on_close_complete frees.
return;
}

Expand Down Expand Up @@ -1753,16 +1757,24 @@ impl WindowsBufferedReader {
if let Some(source) = self.source.take() {
match source {
Source::SyncFile(file) | Source::File(file) => {
// Detach - file will close itself after operation completes.
// Hand the Box off to libuv: detach() leaves either an
// in-flight uv_fs_read (on_file_read) or a scheduled
// uv_fs_close (on_close_complete) pending; the callback
// reclaims the allocation via heap::take. Dropping the
// Box here would free the uv_fs_t out from under libuv.
let raw = bun_core::heap::into_raw(file);
// SAFETY: raw is a live heap File*; the pending fs callback
// is the sole reclaimer (heap::take in on_close_complete).
unsafe { (*raw).detach() };
// is the sole reclaimer (heap::take in on_close_complete /
// on_file_read's detached path) when one is left pending.
unsafe {
if self.flags.contains(WindowsFlags::CLOSE_HANDLE) {
(*raw).detach();
} else if !(*raw).detach_borrowed_fd() {
// Idle and the fd is parent-owned: nothing pending,
// nothing to close. Reclaim and drop the Box.
drop(bun_core::heap::take(raw));
}
}
}
#[cfg(windows)]
Source::Pipe(pipe) => {
Expand Down
22 changes: 5 additions & 17 deletions src/io/PipeWriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1210,24 +1210,12 @@ pub trait BaseWindowsPipeWriter {
// detach() schedules start_close() (now or after the pending
// op completes); on_close_complete heap::take()s `raw`.
(*raw).detach();
} else {
// Don't own fd: stop any in-flight op and detach parent so
// on_fs_write_complete won't touch the (possibly freed)
// writer. We must still reclaim the Box<File>.
(*raw).stop();
(*raw).fs.data = core::ptr::null_mut();
if (*raw).state == crate::source::FileState::Deinitialized {
// No callback will ever fire for this fs_t — sole
// owner, free now.
// SAFETY: `raw` is the Box<File> leaked above via
// into_raw; no libuv request references it.
drop(bun_core::heap::take(raw));
}
// else: state is Operating/Canceling — libuv still owns a
// request pointing into *raw. on_fs_write_complete sees
// parent_ptr null, observes state == Deinitialized after
// complete(), and heap::take()s there.
} else if !(*raw).detach_borrowed_fd() {
// Idle and the fd is parent-owned: nothing pending,
// nothing to close. Reclaim and drop the Box.
drop(bun_core::heap::take(raw));
}
// else: on_fs_write_complete heap::take()s the detached Box.
}
}
Source::Pipe(pipe) => {
Expand Down
9 changes: 9 additions & 0 deletions src/io/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ impl File {
}
}

/// Detach without closing the parent-owned fd. Returns true when an
/// operation is in flight (its callback frees the Box); false when idle
/// (caller drops the Box).
pub fn detach_borrowed_fd(&mut self) -> bool {
self.fs.data = core::ptr::null_mut();
self.stop();
self.state != FileState::Deinitialized
}

/// Mark the operation as complete and clean up.
/// Must be called first in the callback before processing data.
pub fn complete(&mut self, was_canceled: bool) {
Expand Down
77 changes: 77 additions & 0 deletions test/js/bun/http/bun-serve-file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1061,3 +1061,80 @@ process.exit(0);
expect(stdout.trim()).toBe("file-response-started\nstill-serving");
expect(exitCode).toBe(0);
}, 30_000);

// On Windows, FileResponseStream closes its fd via Closer::close in Drop AND
// WindowsBufferedReader::Drop closed the same CRT fd via File::start_close
// (CLOSE_HANDLE was cleared on the reader but never honored). Between the two
// async uv_fs_close calls, an unrelated open could be handed the recycled
// slot and have it closed under it. On POSIX the reader honors CLOSE_HANDLE,
// so this is effectively a Windows regression test.
test.skipIf(!isWindows)(
"Response(Bun.file) does not double-close the fd on Windows",
async () => {
using dir = tempDir("serve-file-double-close", {
"served.bin": Buffer.alloc(32 * 1024, 65),
"victim.json": JSON.stringify({ ok: true }),
"fixture.ts": /* ts */ `
import { openSync, fstatSync, closeSync } from "node:fs";
let serverError: unknown;
const server = Bun.serve({
port: 0,
fetch() {
return new Response(Bun.file("served.bin"));
},
error(e) {
serverError ??= e;
return new Response("err", { status: 500 });
},
});
const url = "http://127.0.0.1:" + server.port + "/";
let canaryHits = 0;
for (let round = 0; round < 160; round++) {
const tasks: Promise<unknown>[] = [];
// Full fetches: each one drops a FileResponseStream on completion.
for (let i = 0; i < 48; i++) tasks.push(fetch(url).then(r => r.arrayBuffer()));
// Aborted fetches: each one drops a FileResponseStream from on_aborted,
// which is where the double-close raced most readily against new opens.
for (let i = 0; i < 48; i++) {
const c = new AbortController();
tasks.push(
fetch(url, { signal: c.signal })
.then(r => { c.abort(); return r.arrayBuffer().catch(() => {}); })
.catch(() => {}),
);
}
// Victim Bun.file().text() reads (async uv_fs_open -> uv_fs_fstat).
for (let i = 0; i < 16; i++) {
tasks.push(Bun.file("victim.json").json().then(v => {
if (!v.ok) throw new Error("wrong contents");
}));
}
await Promise.all(tasks);
if (serverError) throw serverError;
// Canary: a synchronously opened fd must still be valid on the next tick.
// The second queued uv_fs_close runs on the threadpool and, without the
// fix, can close this exact recycled slot.
const canary = openSync("victim.json", "r");
await Bun.sleep(0);
try { fstatSync(canary); } catch { canaryHits++; }
try { closeSync(canary); } catch {}
}
server.stop(true);
if (canaryHits) throw new Error("double-close closed " + canaryHits + " canary fds");
console.log("OK");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "fixture.ts"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "OK", stderr: "", exitCode: 0 });
},
60_000,
);
Loading