node:http2: fix stream id overflow in setNextStreamID and nextStreamID - #37542
Conversation
…reamID Http2Session.setNextStreamID(0.5) passes the JS range check (id > 0) and reaches the native setter as 0, where computing the id to step from subtracted past zero: a panic on debug builds and a wrap to 0xFFFFFFFF on release. The matching step in get_next_stream_id could overflow the other way once a server session was parked at 2 ** 32 - 1. Both steps now saturate. Ids that cannot be stepped back from land on the initial state, and a parked-out id reads back above MAX_STREAM_ID, which the stream-opening callers already reject as out of streams.
|
Status: reproduced on a debug build with Fix and test are in this PR; waiting on CI. |
|
Updated 10:04 AM PT - Aug 11th, 2026
✅ @robobun, your commit 2c1bd68dedc9e7d276a85075308d307e1d9857cb passed in 🧪 To try this PR locally: bunx bun-pr 37542That installs a local version of the PR into your bun-37542 --bun |
WalkthroughChangesThe HTTP/2 frame parser now uses saturating arithmetic for stream ID advancement and rollback. A regression test covers boundary values and verifies a follow-up request. HTTP/2 stream ID handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward saturating-arithmetic fix for a debug-build panic / release-build wrap.
What was reviewed:
- Traced each edge input (0.5→0, 1, 2,
u32::MAX) through the newsaturating_sub/saturating_addpaths for both client and server parity — results match the test's expected values and Node's behavior. - Confirmed the removed
== 0getter arm was unreachable (0 is even, caught byis_multiple_of(2)) and the removed== 1setter special case is subsumed by1.saturating_sub(2) == 0. - Verified all
get_next_stream_id()callers that open a stream (h2_frame_parser.rs:8339,:8772) already reject> MAX_STREAM_ID, so a saturated result is handled as out-of-streams rather than used.
Extended reasoning...
Overview
Two-function change in src/runtime/api/bun/h2_frame_parser.rs: get_next_stream_id and set_next_stream_id swap raw += / -= on a u32 for saturating_add / saturating_sub, and drop two branches made redundant by saturation (an unreachable stream_id == 0 in the client getter — 0 is even so the preceding is_multiple_of(2) arm already covers it — and a == 1 special case in the client setter that existed only to avoid 1 - 2). One new spawned-fixture test in test/js/node/http2/node-http2.test.js exercises both edges.
Security risks
None. This is integer arithmetic on a stream-id counter; the only user-reachable input is a number already range-checked in JS (id <= 0 || id > kMaxStreams). The fix closes a debug panic / release wrap; it does not widen accepted input or change any validation gate.
Level of scrutiny
Low-to-medium. The change is mechanical (four arithmetic ops → their saturating variants) and the removed branches are provably dead or subsumed. I traced every value the test asserts through the new code by hand and confirmed the callers of get_next_stream_id at :8339 and :8772 already reject anything above MAX_STREAM_ID (i32::MAX), so a saturated u32::MAX is handled as "no more streams" rather than being used on the wire. The state.nextStreamID read at :6817 just reports the value, which now stays a large unusable id instead of wrapping to 0.
Other factors
The test follows harness conventions: subprocess spawn with bunExe()/bunEnv, concurrent drain of stdout/stderr/exited, error events wired to process.exit(1), and stderr/parsed-stdout asserted before exitCode. It reaches the server-side setter via Symbol.for("::bunhttp2native::") because ServerHttp2Session doesn't yet expose setNextStreamID — the PR description notes this is tracked separately, and the symbol matches src/js/node/http2.ts:350. The PR description states the fix was verified against the existing node-http2.test.js suite and the four ported Node parallel tests that touch this API.
|
#37553 is stacked on this branch: it makes the native setter ignore ids of the wrong parity, ids not above the current next id and ids above 2 ** 31 - 1 (the nghttp2 rules node ends up with). With the setter bounded, last_stream_id stays within MAX_STREAM_ID on every path, so that PR swaps the saturating step back to plain arithmetic and changes one expectation in the test added here (2 ** 32 - 1 on a server leaves the next id at 2, which is node's value). If this lands first, #37553 rebases down to its own commit; if #37553 lands first, this one is covered by it. |
2 participants
Repro
Debug build:
Release builds wrap instead of panicking. Node (v26) prints
nextStreamID = 1andresponse on stream 1 200: the native call is a no-op there because nghttp2 rejects id 0.Cause
setNextStreamIDinhttp2.tsmirrors node's wrapper (validateNumberplusid <= 0 || id > 2 ** 32 - 1), so a fractional id in (0, 1) is accepted and arrives inH2FrameParser::set_next_stream_idas 0 afterto_u32(). The setter stores the id to step from by subtracting 1 or 2, which underflows for 0 (and the client branch special-cased 1 for the same reason).get_next_stream_idhas the same problem in the other direction: the setter accepts the whole u32 range, and once a server session is parked at2 ** 32 - 1, computing the next id (session.state,pushStream) adds pastu32::MAX:On release that wraps to 0, which
state.nextStreamIDthen reports. Today this second site is only reachable through the native handle, becauseServerHttp2Sessiondoes not exposesetNextStreamID(node does; tracked separately), but it is the same arithmetic.Fix
Both steps use
saturating_sub/saturating_add(src/runtime/api/bun/h2_frame_parser.rs). An id with nothing to step back from lands on the initial state, sosetNextStreamID(0.5)leaves a fresh client on stream 1 like node; a parked-out server reads back as an id aboveMAX_STREAM_ID, whichgetNextStream/requestalready reject as out of streams. The== 1special case in the setter and the unreachable== 0arm in the getter (0 is even) go away with it. No JS change: the wrapper already matches node's validation.Verification
New test
http2 setNextStreamID at the edges of the id space does not overflowintest/js/node/http2/node-http2.test.jsspawns a fixture that sets 0.5 and 1 on a client, 0, 2 and2 ** 32 - 1on a server session, and readsstate.nextStreamIDafter each. Without the fix it aborts with the panics above on a debug build, and on the release binary (USE_SYSTEM_BUN=1) it fails on the server values (0where2and4294967295are expected). With the fix it passes, along with the rest ofnode-http2.test.jsand the portedtest-http2-client-setNextStreamID-errors.js,test-http2-no-more-streams.js,test-http2-client-destroy.jsandtest-http2-session-stream-state.js.[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