Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
68 changes: 44 additions & 24 deletions src/router/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,18 +1123,14 @@ impl Route {
let name_offset = name.as_ptr() as usize - public_path.as_ptr() as usize;
let name_len = name.len();

// NOTE: DirnameStore::append returns `&'static [u8]` (process-
// NOTE: `intern_route_path` returns `&'static [u8]` (process-
// lifetime arena), so rebinding here drops the borrow on
// `route_file_buf` and avoids needing lifetime transmutes
// below.
let dirname_store = FileSystem::instance().dirname_store();
let public_path: &'static [u8] =
dirname_store.append(public_path).expect("unreachable");
let public_path: &'static [u8] = intern_route_path(public_path);
let name: &'static [u8] = &public_path[name_offset..][0..name_len];
let match_name: &'static [u8] = if has_uppercase {
dirname_store
.append_lower_case(&name[1..])
.expect("unreachable")
intern_route_path_lower_case(&name[1..])
} else {
&name[1..]
};
Expand All @@ -1143,9 +1139,7 @@ impl Route {
debug_assert!(name[0] == b'/');
(public_path, name, match_name)
} else {
let dirname_store = FileSystem::instance().dirname_store();
let public_path: &'static [u8] =
dirname_store.append(public_path).expect("unreachable");
let public_path: &'static [u8] = intern_route_path(public_path);
(
public_path,
Route::INDEX_ROUTE_NAME,
Expand Down Expand Up @@ -1231,10 +1225,7 @@ impl Route {
}
};

abs_path_str = FileSystem::instance()
.dirname_store()
.append(_abs)
.expect("unreachable");
abs_path_str = intern_route_path(_abs);

// SAFETY: sole mutation; `base_`/`extname` (which may borrow
// `(*entry).base_.remainder_buf`) are not used after this.
Expand All @@ -1249,11 +1240,7 @@ impl Route {
abs_path_str,
&mut bufs.normalized_abs_path_buf,
);
let interned: &'static [u8] = FileSystem::instance()
.dirname_store()
.append(normalized)
.expect("unreachable");
Interned::from_static(interned)
Interned::from_static(intern_route_path(normalized))
};
#[cfg(not(windows))]
let abs_path = Interned::from_static(abs_path_str);
Expand All @@ -1269,14 +1256,11 @@ impl Route {
}

// NOTE: name/match_name/public_path are already `&'static` via
// DirnameStore::append above. `entry.base()` borrows the entry (it
// `intern_route_path` above. `entry.base()` borrows the entry (it
// may be inline-stored for ≤31-byte names); intern it
// explicitly to get `&'static` without a lifetime transmute.
// SAFETY: read-only reborrow; the `&mut` write above is dead.
let basename: &'static [u8] = FileSystem::instance()
.dirname_store()
.append(unsafe { &*entry }.base())
.expect("unreachable");
let basename: &'static [u8] = intern_route_path(unsafe { &*entry }.base());

Some(Route {
name,
Expand Down Expand Up @@ -1381,6 +1365,42 @@ thread_local! {
#[cfg(windows)]
normalized_abs_path_buf: bun_sys::windows::PathBuffer::ZEROED,
}));

static ROUTE_PATH_INTERN: RefCell<bun_collections::HashMap<&'static [u8], ()>> =
RefCell::new(bun_collections::HashMap::new());
}

/// Intern `value` into the process-lifetime `DirnameStore`, deduplicated by
/// content. `BSSStringList::append` does not dedupe, so without this every
/// `FileSystemRouter::reload()` would re-append each route's public path,
/// absolute path and basename, eventually exhausting the store's slot capacity
/// (`AllocError`) and leaking one heap buffer per append. Same pattern as
/// `intern_transpile_path` in `jsc_hooks.rs`.
fn intern_route_path(value: &[u8]) -> &'static [u8] {
let dirname_store = FileSystem::instance().dirname_store();
if dirname_store.exists(value) {
// SAFETY: `exists` is a pointer-range check against the store's
// process-lifetime backing buffer, so `value` is already `'static`.
return unsafe { core::slice::from_raw_parts(value.as_ptr(), value.len()) };
}
ROUTE_PATH_INTERN.with_borrow_mut(|set| {
if let Some((interned, ())) = set.get_key_value(value) {
return *interned;
}
let interned: &'static [u8] = bun_core::handle_oom(dirname_store.append(value));
set.insert(interned, ());
interned
})
}

fn intern_route_path_lower_case(value: &[u8]) -> &'static [u8] {
if value.len() <= 256 {
let mut scratch = [0u8; 256];
intern_route_path(strings::copy_lowercase(value, &mut scratch[..value.len()]))
} else {
let mut scratch = vec![0u8; value.len()];
intern_route_path(strings::copy_lowercase(value, &mut scratch))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

pub struct Match<'a> {
Expand Down
48 changes: 48 additions & 0 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,54 @@
expect(router.match("/posts")!.name).toBe("/posts");
});

it("reload() does not leak route paths into the process-global intern store", async () => {
// Route::parse interns each route's public path, absolute path, and basename
// into the never-freed DirnameStore. Without content dedup, every reload()
// re-appended identical strings, leaking one heap buffer per append and
// eventually panicking with `unreachable: AllocError` when the store's slot
// capacity was exhausted. Nest the pages directory deeply so the absolute
// path (which dominates bytes) is long enough for the leak to show over the
// unrelated per-reload overhead.
const seg = Buffer.alloc(100, "d").toString();
const parts = Array.from({ length: 14 }, () => seg);
const files: Record<string, string> = {};
for (let i = 0; i < 200; i++) files[path.join(...parts, "pages", `Page${i}.tsx`)] = "export default 1;\n";
using dir = tempDir("fsr-reload-intern", files);
const pagesDir = path.join(String(dir), ...parts, "pages");

Check failure on line 418 in test/js/bun/util/filesystem_router.test.ts

View check run for this annotation

Claude / Claude Code Review

New reload() leak test will fail on macOS and Windows CI due to path construction in tempDir setup

The new `reload()` leak test will fail during `tempDir()` setup on both macOS and Windows CI. On macOS the 14×100-char nested segments produce ~1500-byte absolute paths, exceeding `PATH_MAX=1024` (`mkdirSync`/`writeFileSync` throw `ENAMETOOLONG`, and the router's 1024-byte `PathBuffer` would overflow anyway). On Windows the file-map keys are built with `path.join(...)`, which yields backslash-separated names; `makeTreeSyncFromDirectoryTree` (harness.ts:380) only creates parent directories when t
Comment thread
robobun marked this conversation as resolved.
Outdated

const code = /* ts */ `
const router = new Bun.FileSystemRouter({
dir: ${JSON.stringify(pagesDir)},
style: "nextjs",
fileExtensions: [".tsx"],
});
const m = router.match("/Page7");
if (!m || !m.filePath.endsWith("Page7.tsx")) throw new Error("match() broken: " + m?.filePath);
for (let i = 0; i < 5; i++) router.reload();
Bun.gc(true);
const before = process.memoryUsage.rss();
for (let i = 0; i < 400; i++) router.reload();
Bun.gc(true);
const m2 = router.match("/Page7");
if (!m2 || !m2.filePath.endsWith("Page7.tsx")) throw new Error("match() broken after reload: " + m2?.filePath);
const growthMB = (process.memoryUsage.rss() - before) / 1024 / 1024;
console.error("RSS growth: " + growthMB.toFixed(1) + "MB");
if (growthMB > 110) throw new Error("leaked " + growthMB.toFixed(1) + "MB");
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "--smol", "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("leaked");
expect(stderr).not.toContain("AllocError");
expect(stdout).toBe("");
expect(exitCode).toBe(0);
}, 60_000);

it("reload() works with new dirs/files", () => {
const { dir } = make(["posts.tsx"]);

Expand Down
Loading