FileSystemRouter: deep-copy extensions and asset_prefix_path on reload()#30070
FileSystemRouter: deep-copy extensions and asset_prefix_path on reload()#30070robobun wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Review rate limit: 0/5 reviews remaining, refill in 9 minutes and 19 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #29919. That PR is about the This PR is the focused fix for the |
There was a problem hiding this comment.
LGTM — straightforward arena-lifetime fix that deep-copies the config strings into the new arena before freeing the old one, with a regression test that exercises multiple reloads.
Extended reasoning...
Overview
This PR touches src/bun.js/api/filesystem_router.zig (the reload() method only) and adds one regression test to test/js/bun/util/filesystem_router.test.ts. The change replaces a shallow allocator.dupe(string, extensions) with an explicit loop that dupe(u8, ...)s each inner extension string, and likewise dupes asset_prefix_path, so the new router config is fully self-contained in the new arena before the old arena is freed.
Security risks
None. This is a pure memory-lifetime correction in a local-filesystem routing utility. There is no change to input parsing, auth, permissions, or any externally-reachable surface — it strictly tightens ownership of already-held byte slices.
Level of scrutiny
Low-to-moderate. The diff is ~15 lines of Zig in a single function and follows the exact pattern already used for config.dir one line below. The only new behavior is allocating a few extra small strings into an arena that is freed wholesale on the next reload()/finalize, so there is no leak risk. The catch unreachable and labeled-block style match the surrounding code. The default-extensions case (static string literals) and the empty asset_prefix_path case both degrade to harmless zero/short copies.
Other factors
- The PR description includes an ASAN trace pinpointing the UAF and shows the new test failing before / passing after the fix.
- It is a direct follow-up to #29971 and #29973, which touched the same arena-lifetime area in this file.
- The added test loops
reload()five times and asserts bothroutesandmatch().src(which exercisesasset_prefix_path), giving good coverage of the fix. - No bugs were flagged by the bug-hunting system, and there are no outstanding human reviewer comments.
|
Branch was rebased onto the Build #51003: 25/37 BuildKite jobs Expired waiting for agents across every platform (linux, darwin, windows, freebsd, android). Zero test failures — no tests ran. The 12 jobs that did get agents (build-zig/build-cpp) all passed. Fleet-wide agent shortage, not this change. Previous build (#49815) — 72/75 passed; 3 darwin failures unrelated to this change
|
dylan-conway
left a comment
There was a problem hiding this comment.
@robobun do we have a method or abstraction for deep cloning lists of strings? If so let's use it
|
@dylan-conway Searched — there isn't one.
Kept the inline loop here to match. Happy to add a |
reload() allocated a new arena and called allocator.dupe(string, extensions) to carry the config forward, but that only duplicates the outer []string slice — each inner []const u8 still pointed into the previous arena, which is deinit()ed at the end of reload(). asset_prefix_path was not copied at all. The first reload() worked because loadRoutes runs before the old arena is freed; the second reload() read freed extension bytes in router.zig:443 (strings.eql) for every scanned file, tripping ASAN and silently dropping routes in release. Deep-copy each inner extension string and asset_prefix_path into the new arena so the config is fully self-contained after the old arena is freed. Follow-up to #29971.
6a4d448 to
56f721d
Compare
There was a problem hiding this comment.
LGTM — focused UAF fix that deep-copies arena-backed config slices, with a regression test; ASAN lane is green.
Extended reasoning...
Overview
This PR fixes a use-after-free in FileSystemRouter.reload() (src/runtime/api/filesystem_router.zig). Previously, reload() created a new arena and copied the config forward with allocator.dupe(string, extensions), which only duplicates the outer []string — each inner []const u8 still pointed into the previous arena, which is freed at the end of reload(). asset_prefix_path wasn't copied at all. The second reload() would then read freed bytes when comparing extensions in loadRoutes. The fix replaces the shallow dupe with an explicit alloc + per-element dupe(u8, ext) loop and adds dupe(u8, asset_prefix_path). A regression test in filesystem_router.test.ts calls reload() five times with custom fileExtensions and assetPrefix, asserting routes and match().src remain correct.
Security risks
None. This is internal arena bookkeeping for a developer-facing routing utility. No auth, crypto, network, or untrusted-input parsing is touched. The change strictly removes a memory-safety hazard (UAF) rather than introducing one.
Level of scrutiny
Low-to-medium. The diff is ~12 lines of Zig that follow an established idiom already used across the codebase (test_command.zig, HTMLBundle.zig, StringSet.clone, etc., as the author enumerated in-thread). The new allocations land in the new arena, so they're freed on the next reload()/finalize() with no leak. Edge cases check out: when extensions is the static default_extensions, deep-copying string literals into the arena is harmless; dupe(u8, "") for an empty asset_prefix_path returns an empty slice. The labeled-block + multi-arg for is idiomatic Zig.
Other factors
- Bug-hunting system found no issues.
- CI: 72 lanes passed including
debian-13-x64-asan-test-bun(the lane that would catch this class of bug); the three darwin failures are documented pre-existing flakes unrelated to this change. - The only reviewer question (whether a deep-dupe helper exists) was answered with concrete codebase references; keeping the inline loop matches existing convention and avoids scope creep.
- No CODEOWNERS entry covers this file.
- Overlap with #29919 is acknowledged and non-conflicting — this PR is the more complete fix (also covers
asset_prefix_path) and ships a dedicated regression test.
What
FileSystemRouter.reload()creates a new arena and carries the existing config forward into it before freeing the old arena:allocator.dupe(string, ...)(string=[]const u8) only duplicates the outer[]stringslice — each inner[]const u8still points into the previous arena.asset_prefix_pathisn't copied at all.The first
reload()appears to work becauseloadRoutesruns while the old arena is still live. But the config it leaves behind holds dangling inner slices, so the secondreload()reads freed bytes atrouter.zig:443(strings.eql(extname[1..], _extname)) for every scanned file — tripping ASAN in debug and silently dropping routes in release.Repro
Fix
Deep-copy each inner extension string into the new arena with a loop over
allocator.dupe(u8, ext), anddupe(u8, ...)theasset_prefix_pathas well, so the config is fully self-contained once the previous arena is freed.Verification
git stash -- src/ && bun bd test filesystem_router.test.ts -t 'preserves custom fileExtensions'→ fail (ASAN use-after-poison at router.zig:443)git stash pop && bun bd test filesystem_router.test.ts -t 'preserves custom fileExtensions'→ passfilesystem_router.test.tspass.Follow-up to #29971.