Skip to content
Merged
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
13 changes: 13 additions & 0 deletions src/resolver/dir_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,19 @@ impl DirInfo {
}
}

/// [`get_entries_ref`](Self::get_entries_ref) for call sites that already
/// hold `entries_mutex` (the mutex is non-recursive); see
/// [`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked).
pub fn get_entries_ref_locked(&self, generation: Generation) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at_locked(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(&**entries),
fs::EntriesOption::Err(_) => None,
}
}

pub fn get_entries_const(&self) -> Option<&fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
Expand Down
22 changes: 21 additions & 1 deletion src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1597,14 +1597,34 @@ pub mod fs {

/// Index lookup with generation-check
/// re-read (open + readdir + cache replace) when the cached listing is stale.
///
/// Takes `entries_mutex` for the whole lookup: the generation-stale branch
/// drops the existing `DirEntry` (and the bucket allocation behind its
/// `data` map) in place, and the route loaders iterate that map under the
/// same lock. Call [`entries_at_locked`](Self::entries_at_locked) instead
/// from inside a critical section that already holds `entries_mutex`.
pub fn entries_at(
&mut self,
index: bun_alloc::IndexType,
generation: Generation,
) -> Option<&mut EntriesOption> {
// `MutexGuard` stores the mutex by raw pointer (see `EntriesGuard`),
// so holding it does not keep `&mut self` borrowed.
let _g = self.entries_mutex.lock_guard();
self.entries_at_locked(index, generation)
}

/// [`entries_at`](Self::entries_at) for call sites that already hold
/// `entries_mutex` (the mutex is non-recursive). Currently that is only
/// `dir_info_uncached` when reached from `dir_info_cached_miss`.
pub fn entries_at_locked(
&mut self,
index: bun_alloc::IndexType,
generation: Generation,
) -> Option<&mut EntriesOption> {
// erase to raw immediately so re-borrowing `&mut self` for
// `open_dir`/`readdir`/`read_directory_error` doesn't conflict.
// `entries_mutex` held by caller; sole `&mut` to this slot.
// `entries_mutex` held (by `entries_at` or the caller); sole `&mut` to this slot.
let result_ptr = std::ptr::from_mut::<EntriesOption>(self.entries.at_index(index)?);
// SAFETY: BSSMap-owned slot; uniquely held under `entries_mutex`.
if let EntriesOption::Entries(existing) = unsafe { &mut *result_ptr } {
Expand Down
5 changes: 4 additions & 1 deletion src/resolver/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6201,7 +6201,10 @@

// Make sure "absRealPath" is the real path of the directory (resolving any symlinks)
if !self.opts.preserve_symlinks {
if let Some(parent_entries) = parent_.get_entries_ref(self.generation) {
// The only caller that reaches this with `parent` set
// (`dir_info_cached_miss`) already holds `entries_mutex`, and that
// mutex is non-recursive, so go through the `_locked` accessor.
if let Some(parent_entries) = parent_.get_entries_ref_locked(self.generation) {

Check warning on line 6207 in src/resolver/resolver.rs

View check run for this annotation

Claude / Claude Code Review

dir_info_for_resolution rewrites DirEntry in place without entries_mutex (same bug class)

`dir_info_for_resolution` (resolver.rs:3491–3558) does the same in-place `DirEntry` rewrite this PR fixes — `(*existing).data.clear()` at 3543 and `*p = new_entry` at 3554 — but never takes `entries_mutex` (the SAFETY comment at 3505 claiming it does is stale; the only `entries_mutex.lock_guard()` in this file is at 4269 in `dir_info_cached_miss`). `bust_dir_cache_recursive` snapshots `entries.data.values()` under `entries_mutex` only, so this is the same UAF class, just gated on auto-install (`
Comment thread
robobun marked this conversation as resolved.
if let Some(lookup) = parent_entries.get(base) {
let entries_fd = entries!().fd;
if entries_fd.is_valid()
Expand Down
29 changes: 19 additions & 10 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -836,17 +836,26 @@ it("reload() while Bun.build() resolves the same directory", async () => {
style: "nextjs",
fileExtensions: [".tsx"],
});
const builds = Array.from({ length: 4 }, () =>
Bun.build({ entrypoints, target: "bun", throw: false }),
);
// The first build completes with generation 0 and the bundle thread then
// bumps its generation, so every later build's resolver re-reads the
// directory listing in place. reload() iterates the same listing on the
// main thread, and that in-place re-read is what the reload loop races.
await Bun.build({ entrypoints, target: "bun", throw: false });
let matches = 0;
for (let i = 0; i < 50; i++) {
router.reload();
const m = router.match("/p7");
if (m && m.filePath.endsWith("p7.tsx")) matches++;
let buildsOk = true;
for (let round = 0; round < 40; round++) {
const builds = Array.from({ length: 4 }, () =>
Bun.build({ entrypoints, target: "bun", throw: false }),
);
for (let i = 0; i < 50; i++) {
router.reload();
const m = router.match("/p7");
if (m && m.filePath.endsWith("p7.tsx")) matches++;
}
const results = await Promise.all(builds);
buildsOk &&= results.every(r => r.success);
}
const results = await Promise.all(builds);
console.log("matches", matches, "builds-ok", results.every(r => r.success));
console.log("matches", matches, "builds-ok", buildsOk);
Comment thread
robobun marked this conversation as resolved.
`,
};
for (let i = 1; i <= 40; i++) {
Expand All @@ -863,7 +872,7 @@ it("reload() while Bun.build() resolves the same directory", async () => {
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true");
expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 2000 builds-ok true");
expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null });
}, 60_000);

Expand Down
Loading