FileSystemRouter: keep the percent-decoded path buffer alive for the matched route - #31208
FileSystemRouter: keep the percent-decoded path buffer alive for the matched route#31208alii wants to merge 1 commit into
Conversation
|
@robobun adopt |
|
Updated 2:26 AM PT - May 30th, 2026
✅ @robobun, your commit 3ac5f2e4e6c42d6933a364506b5177a7f2b0a1f2 passed in 🧪 To try this PR locally: bunx bun-pr 31208That installs a local version of the PR into your bun-31208 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR implements percent-decoded buffer ownership transfer through the routing system. ChangesPercent-Decoded Buffer Ownership
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/filesystem_router.test.ts`:
- Line 435: The two percent-encoding tests currently call make(["search.tsx"])
which uses tmpdirSync; replace those calls by creating a temp directory via
using dir = tempDir("percent-encoding-tests", { /* options if needed */ }) and
pass dir: String(dir) into the FileSystemRouter constructor instead of relying
on make(...); update any references to the generated path so the test uses the
created dir variable, keeping the test logic the same and ensuring you
import/use tempDir from the harness and still clean up via the using statement.
🪄 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: 11cadccc-6ecf-4335-be09-f57c9345d099
📒 Files selected for processing (3)
src/http_types/URLPath.rssrc/runtime/api/filesystem_router.rstest/js/bun/util/filesystem_router.test.ts
|
🤖 Rebased onto Note: the use-after-free this PR fixes already landed on Left a comment for @alii to decide: close as already-fixed, or keep the small test tweak. Verified locally (ASAN debug): the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the fix is small, well-reasoned, and the ownership transfer of the decode buffer into MatchedRoute looks correct. That said, this touches unsafe lifetime-erased self-referential storage, so it's worth a human sanity-check.
Extended reasoning...
Overview
This PR fixes a use-after-free in Bun.FileSystemRouter.prototype.match(). When the input URL contains a %, URLPath::parse percent-decodes into a heap Box<[u8]> owned by the stack-local URLPath, and the returned MatchedRoute captures &'static [u8] slices into that buffer. Previously the URLPath (and thus the buffer) was dropped at the end of match(), leaving the route's .pathname/.query/.params slices dangling. The fix adds URLPath::into_decoded_storage() to extract the buffer and a new decoded_path_backing: Option<Box<[u8]>> field on MatchedRoute to own it for the route's lifetime, cleared in deinit. Two regression tests cover percent-encoded query strings and dynamic-segment params.
Security risks
The bug being fixed is itself a memory-safety issue (read of freed memory reachable from JS via any percent-encoded URL passed to router.match()). The fix does not introduce new attack surface — it only extends the lifetime of an existing allocation by moving its owner. No new parsing, no new user-controlled data paths.
Level of scrutiny
High. This is unsafe Rust managing self-referential storage via lifetime erasure (detach_lifetime, 'static slice fields, UnsafeCell), in a Zig→Rust port where ownership semantics shifted from threadlocal to per-instance. The reasoning is sound — moving a Box does not move its heap bytes, into_decoded_storage consumes self so the URLPath can't be double-used, and the buffer is freed exactly once in deinit — but this class of code merits a second pair of eyes rather than bot-only approval.
Other factors
The change is minimal and surgical (one new accessor, one new struct field threaded through init/deinit, comment updates). Argument evaluation order in the MatchedRoute::init(route, path, url_path.into_decoded_storage(), ...) call is fine: route already holds erased-'static slices, and consuming url_path moves (not drops) the backing Box. The added tests directly exercise the previously-UB path. No outstanding reviewer comments.
There was a problem hiding this comment.
The fix looks correct and is well-verified under ASAN, but since it threads ownership through an unsafe self-referential struct with manual lifetime erasure in runtime code, it's worth a human pass on the ownership reasoning.
Extended reasoning...
Overview
This PR fixes a heap-use-after-free in Bun.FileSystemRouter.prototype.match(). When the input URL contains a %, URLPath::parse allocates a percent-decode buffer (_decoded_storage: Option<Box<[u8]>>) and the returned URLPath's slice fields borrow from it. r#match then captures those slices into a RouterMatch that is stored in the long-lived MatchedRoute, but previously dropped the URLPath (and its buffer) at scope exit. The fix adds URLPath::into_decoded_storage(), threads the extracted Box through MatchedRoute::init into a new decoded_path_backing field, and releases it in deinit. Two regression tests cover percent-encoded query strings and dynamic segments, and the existing RSS-leak test's ASAN threshold is widened to include debug builds.
Security risks
The pre-fix state was a read of freed heap memory reachable from user-supplied URLs — a real memory-safety issue. The fix removes that hazard by extending the buffer's lifetime to match the MatchedRoute. I don't see new attack surface introduced: no new parsing, no new external input handling, and the added accessor is a simple ownership move.
Level of scrutiny
This is production runtime code (src/runtime/api/filesystem_router.rs) that already relies heavily on unsafe self-referential patterns: detach_lifetime, 'static lifetime erasure on RouterMatch, raw self-pointers through UnsafeCell, and the invariant that moving a Box does not move its heap bytes. The diff itself is mechanically small (one field, one accessor, one init param, one drop), but its correctness rests on those surrounding invariants — e.g., that route is fully captured before url_path is consumed, that no code path between match_page_with_allocator and MatchedRoute::init can drop the buffer, and that argument-evaluation order doesn't matter because the slices are already lifetime-erased. I believe all of that holds, and the ASAN repro/verify cycle in the PR description is convincing, but this is the kind of ownership reasoning a human maintainer should sign off on rather than a bot.
Other factors
- The one CodeRabbit comment (use
tempDir) was addressed in 5e311b6 and confirmed resolved. - No CODEOWNERS entries match the touched files.
- CI shows failures, but they're all in unrelated files (
process.rs,npm.rs,Futex.rs,bun_core/lib.rs, linker-no-piewarnings) on freebsd/android/musl lanes and not introduced by this diff. - The bug-hunting system found no issues.
|
@robobun rebase and get this mergable |
…Router leak test Debug builds enable ASAN by default, so its quarantine inflates RSS the same way it does for bun-asan; use the existing wide bound there too so the test stays meaningful under a local debug build.
5e311b6 to
3ac5f2e
Compare
|
@alii rebased onto Heads up though: the use-after-free this PR fixes already landed on So the rebase dropped this PR's now-redundant source change. What the branch still carries that is not on Your call:
Verified locally on the rebased branch (ASAN debug): the |
There was a problem hiding this comment.
LGTM — after rebase this is just a one-line test threshold tweak (widen the RSS leak bound for debug builds, which enable ASAN by default).
Extended reasoning...
Overview
After rebasing onto main (where the original use-after-free fix landed independently via #31221), this PR's remaining diff is a single test-only change in test/js/bun/util/filesystem_router.test.ts: the MatchedRoute.params does not leak test's RSS-growth threshold changes from isASAN ? 400 : 20 to isASAN || isDebug ? 400 : 20, plus the corresponding isDebug import from harness and an updated comment. No source code is touched.
Security risks
None. This is a test-only adjustment to a memory-growth threshold in a subprocess-based leak regression test. It does not affect runtime behavior, routing, parsing, or any user-facing surface.
Level of scrutiny
Low. The change loosens a flakiness-prone RSS bound only for debug builds, with a clear and correct rationale: Bun debug builds enable ASAN by default, and ASAN's quarantine retains freed allocations (~256 MB), inflating RSS deltas well past the 20 MB release bound. The strict 20 MB bound is preserved for non-ASAN/non-debug builds, so the test still guards the actual leak on release/CI lanes that matter. isDebug is an existing harness export (Bun.version.includes("debug")).
Other factors
The PR title and description still describe the original (now-superseded) UAF fix, but the timeline makes the post-rebase state explicit and the maintainer asked to get it mergeable as-is. The earlier CodeRabbit comment about tempDir was addressed and resolved. No bugs were found by the bug-hunting system. This is a mechanical, well-justified test reliability tweak.
2 participants
Bun.FileSystemRouter.prototype.match(url)percent-decodes the path into a buffer owned by a stack-localURLPath, then borrows slices of it for the returned route's.pathname/.query/.params. ThatURLPathwas dropped whenmatch()returned, freeing the buffer while theMatchedRoutestill pointed into it — so reading.query/.pathname/.paramson any URL containing a%read freed memory. Plain URLs were unaffected.Move the decoded buffer into
MatchedRoute(a newOption<Box<[u8]>>field), freed once on finalize. Moving theBoxdoesn't move the heap bytes, so the already-captured slices stay valid.The original Zig kept this buffer in a threadlocal; the Rust port switched to a per-
URLPathheap buffer — sounder, but the owner has to travel with the slices. This also closes the threadlocal's latent stale-data window.Adds percent-encoded query + dynamic-param regression tests.
Verification: on a pre-fix ASAN debug build, the new percent-encoding tests abort with
heap-use-after-free(the freed decode buffer is read viaScanner::init); with the fix, the fullfilesystem_router.test.tssuite passes under the ASAN debug build.Also included: the pre-existing
MatchedRoute.params does not leakRSS bound now applies its wide ASAN threshold to debug builds too (debug builds enable ASAN by default, and its quarantine inflates RSS), and the new tests usetempDirfrom the test harness.