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
1 change: 1 addition & 0 deletions src/codegen/generate-js2native.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const rustIdentifierPaths: Record<string, string> = {
"escapeRegExp.rs": "string/escapeRegExp.rs",
"event_loop.rs": "jsc/event_loop.rs",
"ffi.rs": "runtime/ffi/ffi.rs",
"filesystem_router.rs": "runtime/api/filesystem_router.rs",
"h2_frame_parser.rs": "runtime/api/bun/h2_frame_parser.rs",
"hosted_git_info.rs": "install/hosted_git_info.rs",
"http/H2Client.rs": "http/H2Client.rs",
Expand Down
2 changes: 2 additions & 0 deletions src/js/internal-for-testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ const shellParse = $newRustFunction("shell.rs", "TestingAPIs.shellParse", 2);

export const sslCtxLiveCount = $newRustFunction("SecureContext.rs", "jsLiveCount", 0);

export const dirnameStoreAppendCount = $newRustFunction("filesystem_router.rs", "jsDirnameStoreAppendCount", 0);

export const napiThreadsafeFunctionLiveCount = $newRustFunction("napi_body.rs", "jsThreadsafeFunctionLiveCount", 0);

export const escapeRegExp = $newRustFunction("escapeRegExp.rs", "jsEscapeRegExp", 1);
Expand Down
71 changes: 71 additions & 0 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,83 @@
// SAFETY: see `append_slice`.
unsafe { &*$backing() }.exists(value)
}
/// Total number of `append*` calls (inline + overflow). Exposed via
/// `bun:internal-for-testing` so leak tests can assert "N reloads
/// performed zero new appends".
pub fn append_count(&self) -> u32 {
// SAFETY: see `append_slice`.
let b = unsafe { &*$backing() };
let _g = b.mutex.lock();
b.slice_buf_used as u32 + b.overflow_list.count
}

Check warning on line 133 in src/resolver/lib.rs

View check run for this annotation

Claude / Claude Code Review

append_count() forms &BSSStringList over non-UnsafeCell mutable fields before locking

`append_count` forms `let b = unsafe { &*$backing() }` — a whole-struct `&BSSStringList` — before locking, then reads `b.slice_buf_used` / `b.overflow_list.count`, which are plain (non-`UnsafeCell`) fields that `do_append` mutates through `&mut *this` on other threads. That's aliasing UB under Stacked/Tree Borrows and licenses the compiler to hoist the reads above the lock. Mirror the sibling `append`/`append_mutable` pattern instead: `let this = $backing(); let _g = unsafe { (*this).mutex.lock(
Comment thread
robobun marked this conversation as resolved.
}
};
}
string_store_impl!(DirnameStore, DIRNAME_STORE_ZST, dirname_store_backing);
string_store_impl!(FilenameStore, FILENAME_STORE_ZST, filename_store_backing);

thread_local! {
static DIRNAME_INTERN: core::cell::RefCell<
bun_collections::HashMap<&'static [u8], ()>,
> = core::cell::RefCell::new(bun_collections::HashMap::new());
}

impl DirnameStore {
/// Content-deduplicated [`append_slice`]. `BSSStringList::append` does not
/// dedupe, so a bust-then-reread cycle (hot reload, `FileSystemRouter.reload`)
/// would otherwise re-append the same directory path on every miss, exhausting
/// the store's slot capacity and leaking one heap buffer per overflow append.
pub fn intern_slice(&self, value: &[u8]) -> crate::CrateResult<&'static [u8]> {
if self.exists(value) {
// SAFETY: `exists` is a pointer-range check — `value` lies wholly
// within the process-lifetime backing buffer, so widening is sound.
return Ok(unsafe { core::slice::from_raw_parts(value.as_ptr(), value.len()) });
}
DIRNAME_INTERN.with_borrow_mut(|set| {
if let Some((interned, ())) = set.get_key_value(value) {
return Ok(*interned);
}
let interned = self.append_slice(value)?;
set.insert(interned, ());
Ok(interned)
})
}

/// Content-deduplicated [`append_parts`]. See [`intern_slice`].
pub fn intern_parts(&self, parts: &[&[u8]]) -> crate::CrateResult<&'static [u8]> {
const STACK: usize = 512;
let total: usize = parts.iter().map(|p| p.len()).sum();
let mut stack = [0u8; STACK];
let mut heap: Vec<u8>;
let scratch: &mut [u8] = if total <= STACK {
&mut stack[..total]
} else {
heap = vec![0u8; total];
&mut heap[..]
};
let mut at = 0;
for p in parts {
scratch[at..at + p.len()].copy_from_slice(p);
at += p.len();
}
self.intern_slice(&scratch[..at])
}

/// Content-deduplicated [`append_lower_case`]. See [`intern_slice`].
pub fn intern_lower_case(&self, value: &[u8]) -> crate::CrateResult<&'static [u8]> {
const STACK: usize = 256;
let mut stack = [0u8; STACK];
let mut heap: Vec<u8>;
let scratch: &mut [u8] = if value.len() <= STACK {
&mut stack[..value.len()]
} else {
heap = vec![0u8; value.len()];
&mut heap[..]
};
self.intern_slice(bun_core::strings::copy_lowercase(value, scratch))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

macro_rules! string_store_append_impl {
($t:ty, $backing:ident) => {
impl $t {
Expand Down
8 changes: 4 additions & 4 deletions src/resolver/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3509,7 +3509,7 @@ impl<'a> Resolver<'a> {
unsafe { &*existing }.dir
} else {
Fs::file_system::DirnameStore::instance()
.append_slice(dir_path)
.intern_slice(dir_path)
.expect("unreachable")
},
self.generation,
Expand Down Expand Up @@ -4517,9 +4517,9 @@ impl<'a> Resolver<'a> {
let input = &path[..input_path_len];
if input[input.len() - 1] != SEP {
let parts: [&[u8]; 2] = [input, SEP_STR.as_bytes()];
_safe_path = Some(self.fs_ref().dirname_store.append_parts(&parts)?);
_safe_path = Some(self.fs_ref().dirname_store.intern_parts(&parts)?);
} else {
_safe_path = Some(self.fs_ref().dirname_store.append_slice(input)?);
_safe_path = Some(self.fs_ref().dirname_store.intern_slice(input)?);
}
}

Expand Down Expand Up @@ -4578,7 +4578,7 @@ impl<'a> Resolver<'a> {
unsafe { &*existing }.dir
} else {
Fs::file_system::DirnameStore::instance()
.append_slice(dir_path)
.intern_slice(dir_path)
.expect("unreachable")
},
self.generation,
Expand Down
18 changes: 9 additions & 9 deletions src/router/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,17 +1123,17 @@ 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: DirnameStore::intern_* 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");
dirname_store.intern_slice(public_path).expect("unreachable");
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..])
.intern_lower_case(&name[1..])
.expect("unreachable")
} else {
&name[1..]
Expand All @@ -1145,7 +1145,7 @@ impl Route {
} else {
let dirname_store = FileSystem::instance().dirname_store();
let public_path: &'static [u8] =
dirname_store.append(public_path).expect("unreachable");
dirname_store.intern_slice(public_path).expect("unreachable");
(
public_path,
Route::INDEX_ROUTE_NAME,
Expand Down Expand Up @@ -1233,7 +1233,7 @@ impl Route {

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

// SAFETY: sole mutation; `base_`/`extname` (which may borrow
Expand All @@ -1251,7 +1251,7 @@ impl Route {
);
let interned: &'static [u8] = FileSystem::instance()
.dirname_store()
.append(normalized)
.intern_slice(normalized)
.expect("unreachable");
Interned::from_static(interned)
};
Expand All @@ -1269,13 +1269,13 @@ impl Route {
}

// NOTE: name/match_name/public_path are already `&'static` via
// DirnameStore::append above. `entry.base()` borrows the entry (it
// may be inline-stored for ≤31-byte names); intern it
// DirnameStore::intern_slice 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())
.intern_slice(unsafe { &*entry }.base())
.expect("unreachable");

Some(Route {
Expand Down
12 changes: 12 additions & 0 deletions src/runtime/api/filesystem_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ pub use crate::bake::framework_router::JSFrameworkRouter as FrameworkFileSystemR

pub(crate) const DEFAULT_EXTENSIONS: &[&[u8]] = &[b"tsx", b"jsx", b"ts", b"mjs", b"cjs", b"js"];

/// Exposed via `bun:internal-for-testing` so leak tests can assert
/// `reload()` performed zero new `DirnameStore` appends.
#[bun_jsc::host_fn]
pub fn js_dirname_store_append_count(
_global: &JSGlobalObject,
_callframe: &CallFrame,
) -> JsResult<JSValue> {
Ok(JSValue::js_number(
Fs::DirnameStore::instance().append_count() as f64,
))
}

// ── local shims ───────────────────────────────────────────────────────────
// `to_js` lives on the `bun_jsc::ZigStringJsc` extension trait; `from_bytes`
// auto-detects UTF-8.
Expand Down
29 changes: 29 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,35 @@ it("reload() works", () => {
expect(router.match("/posts")!.name).toBe("/posts");
});

it("reload() does not re-intern directory paths into DirnameStore on every bust+reread", () => {
const { dirnameStoreAppendCount } = require("bun:internal-for-testing") as {
dirnameStoreAppendCount: () => number;
};
const files: Record<string, string> = { "pages/sub/nested.tsx": "export default 1;\n" };
for (let i = 0; i < 20; i++) files[`pages/p${i}.tsx`] = "export default 1;\n";
using dir = tempDir("fsr-reload-dirname-intern", files);

const router = new Bun.FileSystemRouter({
dir: path.join(String(dir), "pages"),
style: "nextjs",
fileExtensions: [".tsx"],
});
// First reload primes the intern cache for the bust+reread paths.
router.reload();
const before = dirnameStoreAppendCount();
for (let i = 0; i < 50; i++) router.reload();
const delta = dirnameStoreAppendCount() - before;
// Without the fix each reload re-appended identical bytes into the never-freed
// store: `dir_info_cached_miss` re-interned safe_path + DirEntry.dir per dir
// (6 per reload here) and `Route::parse` re-interned public_path/abs_path/
// basename per route (63 per reload here), so 50 reloads grew the store by
// ~3450 slots and eventually overflowed it with `AllocError`.
expect({ delta, match: router.match("/sub/nested")?.filePath.endsWith("nested.tsx") }).toEqual({
delta: 0,
match: true,
});
});

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

Expand Down
Loading