Conversation
…e Index::init GET /_bun/client/x-ffffffff00000000.js decodes to u32::MAX, the value GenericIndex reserves for Optional::none. on_js_request built the index first and checked the range second, so an assertion build aborted with 'GenericIndex::init: maxInt is reserved for Optional::none'. Check the raw number against route_bundles.len() first.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 31 seconds for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 12:10 PM PT - Sep 18th, 2026
✅ @robobun, your commit ff2e2149b3ee4e1864a026171e0e3f37fc257223 passed in 🧪 To try this PR locally: bunx bun-pr 43329That installs a local version of the PR into your bun-43329 --bun |
|
Status Reproduction, on a build with assertions (
Before the fix, the process aborts with Test: |
There was a problem hiding this comment.
LGTM — a four-line reorder that moves the route_bundles.len() range check ahead of route_bundle::Index::init, so a client-supplied ffffffff index hits the 404 path instead of the debug assert.
What was reviewed:
id as u32/(id >> 32) as u32are exact replacements for the old& 0xFFFFFFFF+try_from().expect()pair (both always fit), andindex < lennow guaranteesindex != u32::MAXbecauseget_or_put_route_bundlebuildsIndex::init(len)before every push.- The sibling
.js.mapbranch inon_js_requestwas checked for the same pattern:source_map_store::Key::initis a plainu64wrapper andSourceId::kind()is a bit test feeding a hashmap lookup, so no sentinel assert is reachable there. - The new test only fails on assertion builds (release already answered 404), which is inherent to a debug-assert fix; it exercises the real script URL first, then both the
u32::MAXsentinel and a near-miss out-of-range index, and confirms the server survives.
Extended reasoning...
Overview
The PR touches src/runtime/bake/DevServer.rs::on_js_request (the /_bun/client/{name}-{index}{generation}.js handler) and adds one devTest to test/bake/dev/html.test.ts. The Rust change reorders two statements: the bounds check against dev.route_bundles.len() now runs on the raw u32 before route_bundle::Index::init is called, and the two u32::try_from(..).expect("int cast") calls become plain as casts. GenericIndex::init (src/bun_core/util.rs:3262) carries a debug_assert!(int != NULL_VALUE), so on debug/ASAN/assertion builds a URL encoding index ffffffff previously aborted the dev server before the range check could reject it.
Security risks
This is a network-reachable unauthenticated path on the dev server, so the change is in scope for a DoS review, and it is a strict improvement: untrusted bytes are now range-checked before being wrapped in the typed index. I confirmed the invariant the new guard relies on — get_or_put_route_bundle (DevServer.rs:5257) constructs Index::init(len) before each push, so len can never exceed u32::MAX and any index < len is a valid, non-sentinel value. I also checked the sibling .js.map branch in the same function for the same class of bug: source_map_store::Key::init is a transparent u64 wrapper with no assert, SourceId::kind() is self.0 & 1, and the result is only used as a hashmap key, so crafted map URLs cannot reach an assert. on_asset_request similarly does a plain hashmap lookup. No auth, crypto, or data-exposure surface is involved; release builds were already returning 404.
Level of scrutiny
Low-to-moderate. The diff is four lines of Rust with a clear mechanical equivalence: id as u32 is bit-identical to u32::try_from(id & 0xFFFFFFFF) on a u64 (the mask guaranteed success), and (id >> 32) as u32 always fits so try_from could never fail there either. The one-line comment explains the ordering constraint that would otherwise be non-obvious to the next reader. No CODEOWNERS entry covers src/runtime/bake/ or test/bake/, and the bug hunt ran to a dry streak without findings.
Other factors
The test cannot fail under USE_SYSTEM_BUN=1 because the system Bun is a release build with debug_assert! compiled out; that is a property of the bug rather than a weakness in the test, and the PR description states this plainly. The test structure is sound: it extracts the real script URL from the served page (so expect(script).toBeString() fails loudly if the URL format ever changes rather than silently sending undefined), verifies the real script serves, probes both the exact sentinel and a near-miss out-of-range value, and re-fetches / to prove the server is still alive. The ?? [] fallback only affects which assertion reports the failure, not whether one does. Two other open PRs reportedly edit lines below this in on_js_request; whichever lands second will need a trivial rebase, which is not a correctness concern for this change.
Problem
GET /_bun/client/x-ffffffff00000000.jsaborts a dev server that has assertions on:panic: GenericIndex::init: maxInt is reserved for Optional::none. A release build answers 404. The request needs no credentials.on_js_request(src/runtime/bake/DevServer.rs:1740). It builtroute_bundle::Index::init(..)from the URL first and checkedroute_bundles.len()second.ffffffffdecodes tou32::MAX, andGenericIndex::init(src/bun_core/util.rs:3262) asserts against that value.src/has 64GenericIndex::initcall sites. Only this one takes bytes from the network.Fix
u32withdev.route_bundles.len()first. Build theIndexonly for a number in range. The twou32::try_from(..).expect("int cast")calls becomeas u32: both inputs always fit.get_or_put_route_bundle(DevServer.rs:5222) growsroute_bundles, and it buildsIndex::init(len)before the push. So a number belowlenis neveru32::MAX.test/bake/dev/html.test.ts. Withsrc/at main,bun bd testfails withDevServer crashed. Also ranesm,bundle,sourcemap,hotanddev-and-produndertest/bake/.Background
/_bun/client/{name}-{index}{generation}.js.indexandgenerationare 8 hex characters each: the four bytes of au32in memory order.GenericIndex<u32, M>is a typed index. It reservesu32::MAXas the none value of its optional form, andinithas adebug_assert!for it.debug_assert!is active in debug, release-asan and release-assertions builds. The builds that ship compile it out.Notes
Not covered. Each input below still aborts the debug build of this branch. Each has an open PR with a test, so this PR leaves them alone.
nandnfoo(SetUrlwith a path that does not start with/) reachdebug_assert!(path[0] == b'/')inFrameworkRouter::match_slow(FrameworkRouter.rs:1255). A secondHframe while a bundle is in flight reachesdebug_assert!(false)(hmr_socket.rs:209).GET http://host/ HTTP/1.1andCONNECT host:port HTTP/1.1reach the samematch_slowassert throughon_request.sM, a wait of more than 1 s, then close givesassertion failed: self.root == v.sMv,sv,sMon one socket givesassertion failed: dev.memory_visualizer_timer.state != EventLoopTimerState::ACTIVE.Inputs that this branch answers without an abort (debug build, server alive after each):
/_bun/client/:x-ffffffff00000000.js,x-feffffff00000000.js,x-ffffffffffffffff.js,x-FFFFFFFF00000000.js, index 0 before the first page load,x-ffffffffffffffff.js.map,x-0000000000000001.js.map,.js.map,.js. All 404. A valid index with a wrong generation answers with the reload stub, as before./_bun/asset/ffffffffffffffff.css,/_bun/asset/zz, a 4000 character asset name: 404./_bun/unrefand/_bun/report_errorwith malformed bodies: 400 or 200.i,u,l,sframes, unknown ids and an empty frame. The server closes the socket or ignores the frame.Other facts
GenericIndexinsrc/, 64::init(call sites. The other 63 take a container length, a loop counter, a constant, or a number that the server packed itself (serialized_failure::Packedmasks to 30 bits).new_route_params(DevServer.rs:6658) takes a number from the framework's own server-side JS, not from the network.scripts/build/config.tssetsassertionstodebug || asanby default, andscripts/build/rust.tsturns ondebug-assertionsfor those builds.u32::MAX as usize >= lenis true, and the answer is 404. SoUSE_SYSTEM_BUN=1 bun test test/bake/dev/html.test.tspasses before and after. The fail-before proof needsbun bd.parse_hex_to_int::<u64>reads all 16 hex characters as the bytes of au64in memory order. On a little-endian host the first group (the index) is the low half and the second group (the generation) is the high half..init(@intCast(id & 0xFFFFFFFF))on aGenericIndex(u30, RouteBundle), then the length check. This was never correct, so the test is in the module's test file and not undertest/regression/.on_js_requestbelow this line and keep the old order. Whichever lands second needs a small rebase.get_or_put_route_bundleis the function that growsroute_bundles), the audit count, and the "Not covered" list. The review also weighed a wider PR that folds in the fixes of Stop HMR websocket frames from aborting or crashing the dev server #33196 and bake: normalize the HTTP request-target before matching framework routes #33232, and rejected it because those PRs already carry them with tests. It suggested a tracking issue for the "Not covered" inputs. That issue is not filed, because each input already has an open PR that records it.