Skip to content
Open
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
63 changes: 47 additions & 16 deletions src/runtime/cli/repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
// ============================================================================

const MAX_HISTORY_SIZE: usize = 1000;
/// Only the tail of a larger history file is read at startup.
const MAX_HISTORY_FILE_BYTES: usize = 4 * 1024 * 1024;
const HISTORY_FILENAME: &[u8] = b".bun_repl_history";

// ANSI escape codes
Expand Down Expand Up @@ -198,26 +200,56 @@
);
self.file_path = Some(Box::<[u8]>::from(path.as_bytes()));

let content: Box<[u8]> = match sys::File::read_from(Fd::cwd(), path) {
sys::Result::Ok(bytes) => bytes.into(),
sys::Result::Err(_) => return Ok(()),
let Some(file) = Self::open_file(path.as_bytes(), sys::O::RDONLY) else {
return Ok(());
};
let Ok(size) = file.get_end_pos() else {
return Ok(());
};
let offset = size.saturating_sub(MAX_HISTORY_FILE_BYTES);
let mut content = vec![0u8; size - offset];
let Ok(len) = file.pread_all(&mut content, offset as u64) else {
return Ok(());
};

for line in strings::split(&content, b"\n") {
// Newest entries are at the end of the file, so walk it backwards and
// stop once MAX_HISTORY_SIZE entries are collected.
let mut rest = &content[..len];
let mut newest_first: Vec<Box<[u8]>> = Vec::new();
loop {
let (before, line) = match strings::rsplit_once_char(rest, b'\n') {
Some(split) => split,
// A tail read starts somewhere inside an older entry.
None if offset > 0 => break,
None => (&[][..], rest),
};
if !line.is_empty() {
self.entries.push(Box::<[u8]>::from(line));
newest_first.push(Box::<[u8]>::from(line));
}
if before.is_empty() || newest_first.len() == MAX_HISTORY_SIZE {
break;
}
rest = before;
}

// Trim to max size
while self.entries.len() > MAX_HISTORY_SIZE {
let _ = self.entries.remove(0);
}
newest_first.reverse();
self.entries = newest_first;

self.position = self.entries.len();
Ok(())
}

/// Without `O_NONBLOCK`, opening a FIFO left at the history path waits for a
/// peer and hangs the REPL before it prints anything. The flag has no effect
/// on the regular file this accepts.
fn open_file(path: &[u8], flags: i32) -> Option<sys::File> {
let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC;
let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?;
match file.kind() {
Ok(sys::FileKind::File) => Some(file),
_ => None,
}
}

Check failure on line 251 in src/runtime/cli/repl.rs

View check run for this annotation

Claude / Claude Code Review

O_NONBLOCK on Windows opens history file as an asynchronous handle, breaking pread/write

On Windows, `sys::O::NONBLOCK` passed to `File::openat` makes `openat_windows_impl` drop `FILE_SYNCHRONOUS_IO_NONALERT` from `NtCreateFile`, so `open_file` now returns an *asynchronous* HANDLE — but `save()`'s `write_all` calls `WriteFile` with `lpOverlapped=NULL` and `load()`'s `pread_all` passes a stack-local `OVERLAPPED` whose SAFETY comment requires a synchronous handle. Before this PR both paths opened synchronous handles, so this regresses Windows history persistence (and the three new non
Comment on lines +244 to +251

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 On Windows, sys::O::NONBLOCK passed to File::openat makes openat_windows_impl drop FILE_SYNCHRONOUS_IO_NONALERT from NtCreateFile, so open_file now returns an asynchronous HANDLE — but save()'s write_all calls WriteFile with lpOverlapped=NULL and load()'s pread_all passes a stack-local OVERLAPPED whose SAFETY comment requires a synchronous handle. Before this PR both paths opened synchronous handles, so this regresses Windows history persistence (and the three new non-skipIf(isWindows) tests round-trip through it). Gate the NONBLOCK | NOCTTY OR-in behind #[cfg(not(windows))] — see DirectoryRoute.rs:319-326 for the same pattern and rationale.

Extended reasoning...

What the bug is

History::open_file unconditionally ORs sys::O::NONBLOCK into the open flags for both the load and save paths:

fn open_file(path: &[u8], flags: i32) -> Option<sys::File> {
    let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC;
    let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?;
    ...
}

The doc comment says "the flag has no effect on the regular file this accepts", which is true on POSIX but false in bun_sys on Windows. On Windows, sys::O::NONBLOCK is 0o4000 (src/sys/lib.rs:1180), and File::openatopenat_aopenat (Windows arm, lib.rs:3969) → openat_windows_aopenat_windows_impl (lib.rs:7137). At line 7153 that function reads let nonblock = (flags & O::NONBLOCK) != 0, and at lines 7182-7186 it does:

let blocking_flag: u32 = if !nonblock {
    w::FILE_SYNCHRONOUS_IO_NONALERT
} else {
    0
};

So passing O::NONBLOCK drops FILE_SYNCHRONOUS_IO_NONALERT from the NtCreateFile options and yields an asynchronous (overlapped) HANDLE.

Why the downstream I/O breaks

Both callers assume a synchronous handle:

  • save()file.write_all()sys::write() (src/sys/lib.rs:3652-3680) calls kernel32::WriteFile with lpOverlapped = null_mut(). Microsoft documents that on a handle opened for asynchronous I/O the caller must supply an OVERLAPPED; passing NULL on such a handle is invalid and in practice fails (there is no I/O-manager-maintained file position). History is therefore silently never saved on Windows.
  • load()file.pread_all()sys::pread() (src/sys/lib.rs:3682-3722) constructs a stack-local OVERLAPPED with hEvent = null and passes it to ReadFile. Its own // SAFETY: comment (line 3702) states "overlapped lives for the synchronous call (handle was not opened FILE_FLAG_OVERLAPPED)", and the error match (lines 3712-3719) handles BROKEN_PIPE/HANDLE_EOF/OPERATION_ABORTED but not ERROR_IO_PENDING. On an asynchronous handle ReadFile may return FALSE + ERROR_IO_PENDING; pread then returns Err and unwinds while the kernel still holds a pointer to the now-dead stack OVERLAPPED — a documented-SAFETY-invariant violation and a stack use-after-free.

Step-by-step proof (Windows)

  1. User launches bun repl on Windows with a .bun_repl_history file present.
  2. History::load calls Self::open_file(path, sys::O::RDONLY).
  3. open_file computes flags = O::RDONLY | O::NONBLOCK | 0 | O::CLOEXEC (O::NOCTTY is 0 on Windows, lib.rs:1228).
  4. File::openat reaches openat_windows_impl; nonblock evaluates true; blocking_flag becomes 0; NtCreateFile is called without FILE_SYNCHRONOUS_IO_NONALERT → the returned HANDLE is asynchronous.
  5. file.pread_all(&mut content, offset) calls sys::pread, which passes the address of a stack OVERLAPPED to ReadFile on that asynchronous handle. Either the read fails immediately (history not loaded) or it returns ERROR_IO_PENDING, in which case pread returns Err and the kernel later writes into freed stack.
  6. On exit, History::save calls Self::open_file(path, O::WRONLY | O::CREAT | O::TRUNC) → same asynchronous handle → WriteFile(..., NULL) fails → history not saved.

Why this is a regression introduced by the PR

Before this PR, save() used sys::open_a(path, O::WRONLY | O::CREAT | O::TRUNC, 0o600) (no NONBLOCK) and load() used sys::File::read_from — both produced synchronous handles on Windows. The PR description states only cargo check -p bun_runtime --target x86_64-pc-windows-msvc was run, not the tests. Three of the new tests — "keeps the newest entries…", "loads a file exactly at the size limit…", "loads only the tail of a file over the size limit" — are not gated by isWindows, set USERPROFILE via homeEnv(), and assert the exact contents of the saved history file, so a broken save() fails Windows CI.

The codebase already documents this hazard

src/runtime/server/DirectoryRoute.rs:319-326 has the exact pattern with a comment explaining why:

// NONBLOCK so opening a FIFO without a writer cannot block the event
// loop on POSIX. Not on Windows: there `openat` maps it to omitting
// FILE_SYNCHRONOUS_IO_NONALERT, which breaks the synchronous reads
// FileResponseStream issues.
#[cfg(not(windows))]
let flags = bun_sys::O::RDONLY | bun_sys::O::CLOEXEC | bun_sys::O::NONBLOCK;
#[cfg(windows)]
let flags = bun_sys::O::RDONLY | bun_sys::O::CLOEXEC;

The unconditional-NONBLOCK sites elsewhere in the tree (Blob.rs:1481, FileReader.rs:206) all go through bun_sys::open(), which on Windows routes to sys_uv::open — a different codepath that does not hit NtCreateFile.

Fix

Gate the POSIX-only flags on cfg:

fn open_file(path: &[u8], flags: i32) -> Option<sys::File> {
    // O_NONBLOCK so opening a FIFO returns immediately on POSIX. Not on
    // Windows: File::openat maps it to omitting FILE_SYNCHRONOUS_IO_NONALERT,
    // which breaks the synchronous pread/write below (and Windows has no
    // filesystem FIFOs anyway).
    #[cfg(not(windows))]
    let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC;
    #[cfg(windows)]
    let flags = flags | sys::O::CLOEXEC;
    let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?;
    ...
}


fn save(&mut self) {
if !self.modified {
return;
Expand All @@ -239,15 +271,14 @@
content.push(b'\n');
}

let file = match sys::open_a(path, sys::O::WRONLY | sys::O::CREAT | sys::O::TRUNC, 0o600) {
sys::Result::Ok(fd) => sys::File::from_fd(fd),
sys::Result::Err(_) => return,
let Some(file) = Self::open_file(path, sys::O::WRONLY | sys::O::CREAT | sys::O::TRUNC)
else {
return;
};
#[cfg(unix)]
let _ = sys::fchmod(file.fd(), 0o600);
match file.write_all(&content) {
sys::Result::Ok(()) => {}
sys::Result::Err(_) => return,
if file.write_all(&content).is_err() {
return;
}

self.modified = false;
Expand Down
116 changes: 113 additions & 3 deletions test/js/bun/repl/repl.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// Tests for Bun REPL
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import { chmodSync, statSync } from "node:fs";
import { mkfifo } from "mkfifo";
import { chmodSync, statSync, truncateSync } from "node:fs";
import path from "path";

// Helper to run REPL with piped stdin (non-TTY mode) and capture output
Expand All @@ -10,7 +11,7 @@ async function runRepl(
options: {
env?: Record<string, string>;
} = {},
): Promise<{ stdout: string; stderr: string; exitCode: number }> {
): Promise<{ stdout: string; stderr: string; exitCode: number; maxRSS: number }> {
const inputStr = Array.isArray(input) ? input.join("\n") + "\n" : input;
const { env = {} } = options;

Expand All @@ -32,7 +33,7 @@ async function runRepl(
const stdout = await new Response(proc.stdout).text();
const stderr = await new Response(proc.stderr).text();

return { stdout, stderr, exitCode };
return { stdout, stderr, exitCode, maxRSS: proc.resourceUsage()!.maxRSS };
}

const stripAnsi = Bun.stripANSI;
Expand Down Expand Up @@ -1266,6 +1267,115 @@ describe.skipIf(isWindows)("REPL history file permissions", () => {
});
});

// Startup reads $HOME/.bun_repl_history and exit rewrites it, both without any
// output, so whatever is at that path must not be able to hang the REPL or
// balloon its memory. See History::load and History::open_file in
// src/runtime/cli/repl.rs.
describe.concurrent("REPL history file loading", () => {
// MAX_HISTORY_SIZE and MAX_HISTORY_FILE_BYTES in src/runtime/cli/repl.rs.
const maxEntries = 1000;
const maxFileBytes = 4 * 1024 * 1024;

const homeEnv = (home: string) => ({ HOME: home, USERPROFILE: home });

// What the REPL printed after the banner: with piped stdin the input is not
// echoed, so this is a "> " prompt per top-level line plus each result.
function session(stdout: string) {
const output = stripAnsi(stdout);
return output.slice(output.indexOf("> "));
}

// Runs one session that evaluates `2 + 2` on top of the given history file
// and returns the lines the REPL saved back. Since only entries that were
// loaded get saved, this shows exactly what was loaded.
async function loadAndSave(home: string) {
const { stdout, stderr, exitCode } = await runRepl(["2 + 2", ".exit"], { env: homeEnv(home) });
expect({ session: session(stdout), stderr, exitCode }).toEqual({ session: "> \n4\n> \n", stderr: "", exitCode: 0 });
const saved = await Bun.file(path.join(home, ".bun_repl_history")).text();
return saved.split("\n");
}

test("keeps the newest entries of a file with more than the maximum", async () => {
const lines = Array.from({ length: maxEntries + 200 }, (_, i) => `entry_${i}`);
using dir = tempDir("repl-history-many", { ".bun_repl_history": lines.join("\n") + "\n" });

// The newest maxEntries lines are loaded; adding `2 + 2` evicts the oldest of them.
expect(await loadAndSave(String(dir))).toEqual([...lines.slice(lines.length - maxEntries + 1), "2 + 2", ""]);
});

// A file larger than the limit is read from the end, and the first line of
// that tail is cut off somewhere in the middle, so it is not an entry.
async function loadAndSaveFileOfSize(prefix: string, size: number) {
const newerEntries = "\nafter_one\nafter_two\n";
const firstLine = Buffer.alloc(size - newerEntries.length, "x").toString();
using dir = tempDir(prefix, { ".bun_repl_history": firstLine + newerEntries });

return (await loadAndSave(String(dir))).map(line => (line === firstLine ? "<first line>" : line));
}

test("loads a file exactly at the size limit in full", async () => {
expect(await loadAndSaveFileOfSize("repl-history-at-limit", maxFileBytes)).toEqual([
"<first line>",
"after_one",
"after_two",
"2 + 2",
"",
]);
});

test("loads only the tail of a file over the size limit", async () => {
expect(await loadAndSaveFileOfSize("repl-history-over-limit", maxFileBytes + 1)).toEqual([
"after_one",
"after_two",
"2 + 2",
"",
]);
});

// Skipped on Windows: node:fs cannot extend a file this far without NTFS
// allocating all of it, whereas on POSIX the extension is a hole.
test.skipIf(isWindows)("does not read a huge file into memory", async () => {
const fileSize = 256 * 1024 * 1024;
using emptyHome = tempDir("repl-history-empty", { ".bun_repl_history": "" });
using hugeHome = tempDir("repl-history-huge", { ".bun_repl_history": "" });
truncateSync(path.join(String(hugeHome), ".bun_repl_history"), fileSize);

const [empty, huge] = await Promise.all([
runRepl([".exit"], { env: homeEnv(String(emptyHome)) }),
runRepl([".exit"], { env: homeEnv(String(hugeHome)) }),
]);

const outcome = ({ stdout, stderr, exitCode }: typeof empty) => ({ session: session(stdout), stderr, exitCode });
expect({ empty: outcome(empty), huge: outcome(huge) }).toEqual({
empty: { session: "> \n", stderr: "", exitCode: 0 },
huge: { session: "> \n", stderr: "", exitCode: 0 },
});
// Reading the whole file would grow the peak by at least fileSize; the
// tail read adds at most maxFileBytes, leaving the rest of the margin for
// run-to-run noise.
expect(huge.maxRSS - empty.maxRSS).toBeLessThan(fileSize / 2);
});

// Skipped on Windows: FIFOs do not exist in its filesystem namespace.
test.skipIf(isWindows)("a FIFO at the history path hangs neither startup nor exit", async () => {
using dir = tempDir("repl-history-fifo", {});
const historyPath = path.join(String(dir), ".bun_repl_history");
mkfifo(historyPath);

// Nothing ever opens the other end of the FIFO: a blocking open() while
// loading history at startup, or while saving the `1 + 1` entry at exit,
// hangs the REPL until the test times out.
const { stdout, stderr, exitCode } = await runRepl(["1 + 1", ".exit"], { env: homeEnv(String(dir)) });

expect({ session: session(stdout), stderr, exitCode, stillFifo: statSync(historyPath).isFIFO() }).toEqual({
session: "> \n2\n> \n",
stderr: "",
exitCode: 0,
stillFifo: true,
});
});
});

// `bun --interactive` boots the full node:repl + readline + acorn stack; on a
// debug+asan build that is ~4–5s per spawn, so the 5s default is too tight.
const interactiveTimeout = 20_000;
Expand Down