FileSystemRouter: build JS error before freeing arena in error paths - #29971
Conversation
The constructor's log is initialized with the arena allocator. When route loading produces errors (e.g. a filename like '[foo.tsx' missing its closing bracket), the arena was freed before log.toJS() read the messages, causing a use-after-free that ASAN catches as use-after-poison. Build the JS error value first, then free the arena. Applies to all four log.toJS() call sites in constructor() and reload().
|
Updated 8:54 PM PT - Apr 30th, 2026
❌ @robobun, your commit 833d992 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29971That installs a local version of the PR into your bun-29971 --bun |
|
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 (1)
WalkthroughThe PR adjusts error-throwing logic in FileSystemRouter to compute JS error messages before freeing memory, and adds a regression test for malformed route filenames. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 4 minutes and 40 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/filesystem_router.zig`:
- Around line 112-117: The current code calls log.toJS(...) before cleaning up
arena-backed resources so if log.toJS returns error.JSError the subsequent
origin_str.deinit(), arena.deinit(), and globalThis.allocator().destroy(arena)
are skipped and leak; modify the error path in the block containing log.toJS,
origin_str.deinit, arena.deinit, globalThis.allocator().destroy, and
globalThis.throwValue so that cleanup always runs (use defer or capture the
error first, perform origin_str.deinit()/arena.deinit()/destroy(arena), then
convert/throw the JS error), ensuring log.toJS and throwValue still operate but
only after deterministic cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ed183d6-5f20-45f2-b74d-3e51caf3c468
📒 Files selected for processing (2)
src/bun.js/api/filesystem_router.zigtest/js/bun/util/filesystem_router.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward reordering to build the JS error before freeing the arena that backs the log; left one optional drive-by note about an adjacent pre-existing leak.
Extended reasoning...
Overview
This PR fixes a use-after-free in FileSystemRouter's constructor and reload() error paths. The error log is initialized with the arena allocator, but the previous code did arena.deinit() before calling log.toJS(), which iterates log.msgs.items (arena-backed). The fix hoists log.toJS() above the arena teardown at four call sites and stores the result in a local before throwing. A subprocess-isolated regression test is added that triggers the route-loading error path with an invalid filename ([foo.tsx).
I verified that Log.toJS (src/logger.zig:725) dispatches to BuildMessage.create / ResolveMessage.create, both of which msg.clone(allocator) into globalThis.allocator() (src/bun.js/BuildMessage.zig:54, src/bun.js/ResolveMessage.zig:174), so the returned err_value does not retain any pointers into the arena and is safe to throw after arena.deinit() / destroy(arena).
Security risks
None. This is a memory-safety fix in an error path; no new inputs, parsing, auth, or external surface is introduced.
Level of scrutiny
Low–medium. The change is a mechanical reordering of three statements at four sites, each following the identical pattern. The PR description includes ASAN before/after verification, and a new test asserts both a clean exit and the exact error message. No CODEOWNERS cover this path.
Other factors
The one inline comment is explicitly pre-existing (the readDirInfo catch in reload() leaks the new arena — it never had the UAF because it never freed the arena at all). It's a reasonable drive-by suggestion but orthogonal to this fix and shouldn't block. origin_str is allocated via globalThis.allocator(), not the arena, so its deinit ordering relative to log.toJS is immaterial.
Capture the error union instead of using try, so arena.deinit() and destroy(arena) run unconditionally before the result is unwrapped.
Pre-existing leak: the readDirInfo catch block in reload() returned without freeing the newly-created arena. Apply the same pattern as the other four log.toJS sites.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/bun.js/api/filesystem_router.zig:255-257— 🟣 Pre-existing, same arena-lifetime UAF class, three lines above this hunk:allocator.dupe(string, this.router.config.extensions)only copies the outer[]([]const u8)— the inner string bytes still point into the old arena, whichthis.arena.deinit()frees a few lines later on the success path. On the nextreload(),loadRoutesdereferences those freed extension bytes (for (this.config.extensions) |_extname| if (strings.eql(extname[1..], _extname)), src/router.zig:442-443). Only fires whenfileExtensionswas passed (default_extensions is comptime-static), which is why the existing reload() tests miss it. A deep-dupe of each extension string into the new arena before freeing the old one would close it.Extended reasoning...
What the bug is
reload()shallow-copiesthis.router.config.extensionsinto the new arena, then frees the old arena that owns the underlying string bytes. The new router'sconfig.extensions[i].ptrare left dangling, and the nextreload()reads them insideloadRoutes.This is pre-existing — the PR doesn't touch line 254 or change when the old arena is freed on the success path — but it's the same arena-lifetime UAF class this PR is fixing, sitting three lines above the
reload()hunk you just modified, so it's worth flagging while you're here.The specific code path
Allocation. In the constructor, when
fileExtensionsis passed:const allocator = arena.allocator(); ... extensions.appendAssumeCapacity((try val.toUTF8Bytes(globalThis, allocator))[1..]);
JSValue.toUTF8Bytesis documented "The returned slice is always owned byallocator" (src/bun.js/bindings/JSValue.zig:1259-1264), so each extension's bytes live in the constructor's arena.router.config.extensions = extensions.itemsis then stored on the router, and that arena becomesthis.arena.Shallow copy. In
reload()(line 254, immediately above this hunk):.extensions = allocator.dupe(string, this.router.config.extensions) catch unreachable,
where
string = []const u8.std.mem.Allocator.dupeon a[]const []const u8allocates a new outer array and@memcpys the elements verbatim — i.e., it copies the{ptr, len}slice headers. The inner.ptrvalues still point into the old arena.Free. On the success path a few lines later:
this.arena.deinit(); // frees the old arena -> extension bytes are gone globalThis.allocator().destroy(this.arena); this.arena = arena; this.router = router; // stores config.extensions whose [i].ptr now dangle
Dereference. On the next
reload(),router.loadRoutes→RouteLoader.loaditerates each directory entry and does:for (this.config.extensions) |_extname| { if (strings.eql(extname[1..], _extname)) { // src/router.zig:442-443
_extnameis one of the dangling slices;strings.eqlreads its bytes — use-after-free.Why existing code doesn't prevent it
Router.initstoresconfigverbatim with no deep copy, and nothing betweenthis.arena.deinit()and the nextloadRoutesre-materializes the extension bytes..diron the line above is deep-duped (allocator.dupe(u8, ...)), but.extensionsis a slice-of-slices and only gets the outer level duped.The two existing
reload()tests don't passfileExtensions, so they take thedefault_extensionsbranch — those are comptime-static string literals that survivearena.deinit(), masking the bug.Impact
For any router constructed with an explicit
fileExtensionsoption, the second (and every subsequent)reload()reads freed memory while matching file extensions. In ASAN/debug builds this is a use-after-poison crash; in release it can silently mis-match extensions or crash depending on what the freed page now contains.Step-by-step proof
new Bun.FileSystemRouter({ dir, style: "nextjs", fileExtensions: [".tsx"] })→ constructor allocates the bytes"tsx"in arena A viatoUTF8Bytes(globalThis, arena_A.allocator());this.router.config.extensions = [ {ptr→A, len=3} ];this.arena = A.- First
router.reload():- Creates arena B.
.extensions = B.dupe(string, [ {ptr→A, len=3} ])→ new outer array in B, element still{ptr→A, len=3}.loadRoutesruns against{ptr→A}while A is still live → fine.- Success path:
this.arena.deinit()frees A;this.arena = B;this.router.config.extensions = [ {ptr→A(freed), len=3} ].
- Second
router.reload():- Creates arena C.
.extensions = C.dupe(string, [ {ptr→A(freed), len=3} ])→ still{ptr→A(freed)}.loadRoutes→ src/router.zig:443strings.eql(extname[1..], _extname)reads 3 bytes atptr→A→ UAF.
How to fix
Deep-dupe each inner string into the new arena before freeing the old one, e.g.:
.extensions = blk: { const old = this.router.config.extensions; const new = allocator.alloc(string, old.len) catch unreachable; for (old, new) |s, *out| out.* = allocator.dupe(u8, s) catch unreachable; break :blk new; },
(or skip the copy entirely when
old.ptr == default_extensions.ptr).
|
CI failures are all pre-existing flakes unrelated to this change. Build 49359 (6cd9f81, pre-sync) — cross-referenced against builds 49358/49357/49354/49351:
Build 49546 (833d992, post-sync) — cross-referenced against builds 49540-49545:
Re: the additional finding about |
There was a problem hiding this comment.
LGTM — straightforward UAF fix (build error before freeing the arena that backs it), applied consistently with a regression test; the inline note below is pre-existing and fine as a follow-up.
Extended reasoning...
Overview
This PR fixes an ASAN-confirmed use-after-free in Bun.FileSystemRouter's error paths. The Log is initialized with the arena allocator, but the old code did arena.deinit() before calling log.toJS(), so log.msgs.items was read after being freed. The fix is mechanical and applied identically at five sites across constructor() and reload(): capture log.toJS(...) into an error-union local first, then run cleanup (origin_str.deinit() / arena.deinit() / destroy(arena)), then return globalThis.throwValue(try err_value). Log.toJS hands the messages to BuildMessage.create / ResolveMessage.create with globalThis.allocator(), so the resulting JSValue does not retain arena-backed memory. A subprocess regression test asserts the expected "Route is missing a closing bracket]" error with empty stderr and exit 0.
Two earlier review rounds were already addressed: CodeRabbit's note that try log.toJS before cleanup would skip cleanup on JSError (fixed in ed3f8bd by capturing the union and deferring try until after cleanup), and my own note that reload()'s readDirInfo catch leaked the freshly-allocated arena (fixed in 6cd9f81). All inline threads are resolved.
Security risks
None. This touches only error-path resource ordering in a dev-time filesystem router; no auth, crypto, network, or untrusted-input parsing semantics change.
Level of scrutiny
Low–medium. The diff is ~30 lines of reorder-and-rename across five near-identical blocks plus comments, with no new control flow beyond what reviewers already requested. The added test follows the file's existing bunExe() + bunEnv subprocess pattern (where bunEnv suppresses ASAN startup noise, so expect(stderr).toBe("") is the established convention here).
Other factors
The one new inline comment is explicitly flagged pre-existing: reload() lacks the post-loadRoutes log.errors + log.warnings > 0 check that the constructor has, so logged-only validation errors are silently swallowed on reload. That asymmetry predates this PR and is a reasonable follow-up, not a blocker. CI showed build-zig failures on the intermediate commit ed3f8bd, but the head commit 6cd9f81 supersedes it and the final source compiles to the same pattern used elsewhere in the file; merge is CI-gated regardless. No CODEOWNERS cover these paths.
There was a problem hiding this comment.
LGTM — straightforward reordering fix for a real ASAN-detected UAF, with a regression test; the deferred reload() log-check is reasonably out of scope.
Extended reasoning...
Overview
This PR fixes a use-after-free in FileSystemRouter.constructor() and reload() (src/bun.js/api/filesystem_router.zig). The error log is backed by an arena allocator; the old code freed the arena and then called log.toJS(), which reads arena-backed msgs.items. The fix captures log.toJS(...) into an error-union local before arena.deinit() / destroy(arena), then unwraps with try after cleanup — applied uniformly across all five log.toJS sites in the two functions. A subprocess regression test in filesystem_router.test.ts exercises the [foo.tsx missing-bracket path and asserts the correct error message with empty stderr and exit 0.
Security risks
None. This is a pure lifetime/ordering fix in error paths; no new inputs, parsing, auth, or external surface area.
Level of scrutiny
Low-to-moderate. The change is mechanical (reorder three statements at each site), the pattern is identical at every site, and it's confined to error-handling paths that previously crashed under ASAN. The error-union capture ensures cleanup runs even if toJS itself returns error.JSError, addressing the earlier CodeRabbit concern.
Other factors
- All inline review threads are resolved. CodeRabbit's cleanup-on-
toJS-failure concern was addressed in ed3f8bd; my pre-existingreload()readDirInfoarena leak note was addressed in 6cd9f81; my note about the missinglog.errors > 0check inreload()was reasonably deferred as a behavioral change for a follow-up PR. - The author triaged CI failures as pre-existing flakes unrelated to this change, and
filesystem_router.test.tsis 19/19 green on the ASAN lane. - No CODEOWNERS cover the touched paths.
- The bug-hunting system reported no findings on the current revision.
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.
…ven-sh#29971) ## What `FileSystemRouter`'s constructor (and `reload()`) initialize the error log with the arena allocator: ```zig const allocator = arena.allocator(); ... var log = Log.Log.init(allocator); ``` When route loading produces errors, the error paths did: ```zig arena.deinit(); globalThis.allocator().destroy(arena); return globalThis.throwValue(try log.toJS(...)); // reads arena-backed msgs.items ``` `log.msgs.items` is backed by the arena, so `log.toJS()` reads freed memory. ASAN reports `use-after-poison` in `logger.Log.toJS`. ## Repro ```js // pages/[foo.tsx — missing closing bracket new Bun.FileSystemRouter({ style: "nextjs", dir: "./pages", fileExtensions: [".tsx"] }); ``` Debug (ASAN) build: ``` AddressSanitizer: use-after-poison ... oven-sh#1 in logger.Log.toJS (src/logger.zig:733) oven-sh#2 in FileSystemRouter.constructor (src/bun.js/api/filesystem_router.zig:149) ``` ## Fix Build the JS error value first (while the arena is still live — `BuildMessage.create` / `ResolveMessage.create` clone the msg into `globalThis.allocator()`), then free the arena, then throw. Applied to all four `log.toJS()` call sites across `constructor()` and `reload()`. ## Verification - `git stash -- src/ && bun bd test filesystem_router.test.ts -t 'invalid route'` → **fail** (ASAN crash in subprocess) - `git stash pop && bun bd test filesystem_router.test.ts -t 'invalid route'` → **pass**, error message is `Route is missing a closing bracket]` - All 19 existing `filesystem_router.test.ts` tests pass. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
What
FileSystemRouter's constructor (andreload()) initialize the error log with the arena allocator:When route loading produces errors, the error paths did:
log.msgs.itemsis backed by the arena, solog.toJS()reads freed memory. ASAN reportsuse-after-poisoninlogger.Log.toJS.Repro
Debug (ASAN) build:
Fix
Build the JS error value first (while the arena is still live —
BuildMessage.create/ResolveMessage.createclone the msg intoglobalThis.allocator()), then free the arena, then throw. Applied to all fourlog.toJS()call sites acrossconstructor()andreload().Verification
git stash -- src/ && bun bd test filesystem_router.test.ts -t 'invalid route'→ fail (ASAN crash in subprocess)git stash pop && bun bd test filesystem_router.test.ts -t 'invalid route'→ pass, error message isRoute is missing a closing bracket]filesystem_router.test.tstests pass.