Repository navigation
Conversation
Each route reads its parameter names from one vector that holds the names of every route of the server. The range into that vector had a 16-bit start and a 16-bit count, and both were cast down with no check. With 65,536 names in the routes before it, a route built `req.params` with the names of another route. A route with 65,536 names of its own lost them. Store the start and the count as `uint32_t`, the index type of the vector. The brace initializer now takes both values with no cast, so a narrowing conversion does not compile. `getParameter()` takes a 16-bit index, so a name past that index gets the empty string and not the value of a wrapped index. The table of ranges had two entries per route and used one. It now has one entry per route, so it stays at 8 bytes per route.
|
Updated 12:40 PM PT - Oct 13th, 2026
❌ @robobun, your commit a375ba2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 45174That installs a local version of the PR into your bun-45174 --bun |
|
Reproduced on bun 1.4.3-canary.1 (Linux x64) with the reported script: 1,024 routes of 64
PR: #45174 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughRoute identifier ranges now use 32-bit start and count fields. Parameter lookup calls uWS only for indices at or below ChangesRoute Parameter Indices
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes per-request native code in the Bun.serve routing path and the one remaining question (JSC's MarkedArgumentBuffer behaviour past 65,536 entries) can't be confirmed from this checkout, a human look would still be worthwhile.
What was reviewed:
- The
IdentifierRangewidening and the dropped*2sizing:m_pathIdentifierRangesis only ever indexed by the route index (same bound asm_routes), andfinishCreationwrites exactlypaths.size()entries, so the old oversizing masked nothing. - The
i <= numeric_limits<unsigned short>::max()guard againstpackages/bun-uws/src/HttpParser.h:560andHttpRouter.h:add()truncates patterns atMAX_URL_SEGMENTS(100) andgetParameterreturns empty forindex > paramsTop, so names past index 65,535 can never have a value and""is the correct result. - The new test's shape: the 65,538-segment route registers at depth 100 in the uWS trie, so the 100-segment request does match it and
a65536is the first index past 16 bits;m_pathIdentifiers.size()is a WTF::Vector size and fitsuint32_t. - Not verifiable here: that
MarkedArgumentBuffer::ensureCapacity/appendhave no 0x10000 cap (JSC headers aren't in the checkout); the test assertsargs.at(65536)directly, so a debug/ASAN CI run of it settles this.
Extended reasoning...
The diff touches src/jsc/bindings/ServerRouteList.cpp (widens IdentifierRange start/count from uint16_t to uint32_t, sizes the range table to paths.size(), and guards getParameter() calls to indices that fit unsigned short) and adds two tests in test/js/bun/http/bun-serve-routes.test.ts. It touches no auth, crypto, or injection surface; the untrusted-input surface is the request path, which uWS already caps at 100 segments. The logic is small and I confirmed the boundary semantics against the vendored uWS router and parser, and no CODEOWNERS entry covers the changed files. I chose defer over approve because the per-request loop now runs up to the full identifier count with no upper bound and the MarkedArgumentBuffer growth semantics at 65,537 entries could not be checked from the checkout, so the CI debug/ASAN run of the new test is the deciding evidence a human should see.
|
The open question about
|
Problem
Bun.serve({ routes })givesreq.paramsthe names of another route once the routes before it hold 65,536:namesegments./last/:id/:sluganswers{"k0_0":"7","k0_1":"x"}.IdentifierRange(src/jsc/bindings/ServerRouteList.cpp:41) stores auint16_tstart and count.finishCreationcasts a server-wide name index into it (:152).countwraps too: one route with 65,536 names getsreq.params={}.Fix
startandcountare nowuint32_t, the index type of the vector. With no cast in the initializer, a narrowing conversion does not compile.getParameter()takes a 16-bit index. A name past it gets"", not the value of the wrapped index.test/js/bun/http/bun-serve-routes.test.ts(two new tests, bun 1.4.3-canary.1 fails both). Alsoserve-http3.test.ts.Background
ServerRouteListis the C++ object behindroutes. One vector holds the parameter names of all routes, with a range per route.req.paramslists a name past that as"", so the count of one route has no bound.Bun.serve()past 65,535 names: it rejects a working table. Considered a widergetParameter()index: two uWS headers change to save one compare.Downsides
llvm-objdump,-O3, no LTO). The object code of the file is 58 bytes smaller.Notes
Runs of the reported script. It has 1,024 routes of 64 names, then
/last/:id/:slug. The table showsreq.paramsofGET /last/7/xby the count of names in front of the last route.{"k0_0":"7","k0_1":"x"}{"k0_1":"7","k0_2":"x"}{"k0_0":"7","k0_1":"x"}One route with N names. The request is
/x/v0/.../v98. The table shows the key count ofreq.params. In each run of this PR the namesa0toa98have a value and every other name is"".The guard in the loop. A build with both fields widened and the
unsigned shortcast left alone answersa65536: "v0"in the second test. Index 65,536 wraps to 0.Cost numbers. I compiled
ServerRouteList.cppat the merge base and at this PR with the release flags (-O3, without-flto=thin, against the headers of the debug build directory). I compared the two objects withllvm-nm -Sandllvm-objdump -d.movzx,cmp,jl. With this PR:cmp,ja,cmp,jl.paramsObjectForRoute(each of its two instantiations): 1,325 to 1,309 bytes.create: 355 to 299 bytes.finishCreation: 1,717 to 1,747 bytes. Text of the object: 18,236 to 18,178 bytes.perfandvalgrindare not installed here, so the instruction count comes from the disassembly and not from a run.Test shape. The tests use one route with 65,537 names, not 1,024 routes.
Bun.serve()takes 16 s to register 1,024 routes of 64 names in a debug ASAN build (#35248 and #30410 are open for the registration time). The two tests add about 3.5 s to the file in a debug ASAN build.Suites run with the debug ASAN build:
test/js/bun/http/bun-serve-routes.test.ts(63 pass) andtest/js/bun/http/serve-http3.test.ts(77 pass).[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file