Conversation
The goaway() binding read the error code with to_int32(), which saturates doubles outside the i32 range, so any code in (0x7fffffff, 0xffffffff] went out on the wire as 0x7fffffff. The GOAWAY error code is an unsigned 32-bit field and node transmits the argument via Uint32Value, so read it with to_u32() like rst_stream() already does.
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 30 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status Reproduced on Bun 1.4.3-canary (367d939, the same read as main faac63e) with the script in the PR description: The description has the table of codes against Node. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line coercion fix (to_int32 → to_u32) so GOAWAY error codes >2^31-1 reach the wire unchanged.
What was reviewed:
- Confirmed
to_u32()clamps into[0, u32::MAX]and cannot throw after theis_number()guard;ErrorCodeisCopyso building it once is fine. - Matches the existing
rst_stream()pattern at h2_frame_parser.rs:7350 which already reads its error code withto_u32(). - Test covers both session types × both send paths (with/without opaqueData), awaits the actual
goawayevent, uses port 0, and cleans up infinally.
Extended reasoning...
Overview
Single-line semantic change in H2FrameParser::goaway() (src/runtime/api/bun/h2_frame_parser.rs): the JS errorCode argument is now read with JSValue::to_u32() instead of to_int32(), and the ErrorCode newtype is constructed once instead of at each send_go_away call site. The rest of the diff is a mechanical consequence of hoisting the ErrorCode(...) construction. A four-case parameterized test is added to test/js/node/http2/node-http2.test.js alongside the existing goaway test.
Security risks
None. This changes how a numeric argument already validated by the JS layer is coerced before being written into a fixed-width u32 protocol field. No new inputs are accepted, no bounds change on the wire (the field was always 4 bytes), and the receive path is untouched.
Level of scrutiny
Low. The root cause is well-explained and verifiable: to_int32() saturates doubles above i32::MAX (JSValue.rs:759), so 0xffffffff became 0x7fffffff before the as u32 cast. to_u32() clamps into the full [0, u32::MAX] range (JSValue.rs:784-785) via to_int64(), which is exactly what RFC 9113 §6.8's unsigned 32-bit error-code field needs. The is_number() check immediately above means neither helper can throw. ErrorCode is #[derive(Copy)] (line 282), so passing the hoisted value to two call sites is sound. The sibling rst_stream() method already reads its error code with to_u32() (line 7350), so this brings goaway() in line with the file's own convention.
Other factors
The test is well-constructed per repo guidelines: it.each over the 2×2 matrix (server/client sender × with/without opaqueData, hitting both send_go_away branches), Promise.withResolvers to await the actual goaway event, port 0, try/finally cleanup registered before the assertion, and a single toEqual on {code, lastStreamID, opaqueData}. The client's expected ERR_HTTP2_SESSION_ERROR on receiving a non-zero GOAWAY is swallowed. The PR body documents that all four cases fail on the released binary (receiving 2147483647) and pass on the debug build, and that the existing 351 tests in the file plus the ported Node goaway suites still pass. The one-line comment on the changed line explains why (i32 saturation) rather than what, which is the repo's bar. No prior reviews or outstanding comments to address.
|
Updated 4:27 PM PT - Aug 11th, 2026
✅ @robobun, your commit 91c2fbdc9a9533e8e644b3c2f024f1b8a631b105 passed in 🧪 To try this PR locally: bunx bun-pr 37558That installs a local version of the PR into your bun-37558 --bun |
There was a problem hiding this comment.
LGTM — one-line fix swapping a saturating signed read for the unsigned one the GOAWAY error-code field needs.
What was reviewed
- Confirmed
to_u32()is non-throwing after theis_number()guard and matches the sibling error-code reads inrst_stream()andemit_error_to_all_streams(). - Checked
send_go_awayalready takesErrorCode(u32), so the two call-site simplifications are pure refactor. - Test: wires session error/close on both ends to reject (no timeout-only failure), covers both session classes × both send paths, uses
port: 0and cleans up infinally.
Extended reasoning...
Overview
This PR changes one argument read in H2FrameParser::goaway() (src/runtime/api/bun/h2_frame_parser.rs) from to_int32() to to_u32(), wraps it directly in ErrorCode(...), and simplifies the two send_go_away call sites that previously did ErrorCode(error_code as u32) inline. It adds a 4-case it.each test to test/js/node/http2/node-http2.test.js that sends 0xffffffff and 0x80000000 from both a server session and a client session and asserts the peer's 'goaway' event delivers the code, lastStreamID, and opaque data unchanged.
Security risks
None. This widens the range of an outbound protocol field from a saturated i32 to the full u32 the RFC 9113 GOAWAY frame defines. No untrusted-input parsing changes; the receive path was already u32. The value is caller-supplied and JS-layer-validated (validateInteger(code, "code", 0, kMaxUint32) on the server side).
Level of scrutiny
Low. The effective change is a single-token substitution (to_int32 → to_u32) on a value already guarded by is_number(). to_u32() is self.to_int64().clamp(0, u32::MAX as i64) as u32 — non-throwing on numbers, and the identity on [0, 2^32-1], which is exactly the range the JS wrapper validates. The two other error-code reads in this file (rst_stream at line 7349, emit_error_to_all_streams at line 8698) already use to_u32(), so this brings goaway() in line with its siblings. The send_go_away signature already takes ErrorCode, so hoisting the wrapper to the read site is a no-op refactor.
Other factors
The test follows repo conventions well: it.each for the matrix, port: 0, error/close events on both sessions wired to reject so a lost frame fails with a cause instead of timing out (the second commit on the PR is exactly this hardening), exact toEqual assertion on {code, lastStreamID, opaqueData}, and cleanup in finally. Post-resolve reject() calls from the close events fired during finally are no-ops on an already-settled promise. The PR description documents that all four cases fail on the released binary (receiving 2147483647) and pass on the debug build, and that the rest of the file plus the ported Node goaway/shutdown tests still pass. No prior reviewer comments to address.
There was a problem hiding this comment.
LGTM — one-line signed→unsigned read fix that brings goaway() in line with its sibling error-code readers.
What was reviewed:
- Confirmed
to_u32()clamps viato_int64().clamp(0, u32::MAX)(src/jsc/JSValue.rs:784), which is the identity over the[0, 2^32-1]range the JS layer validates. - Verified
rst_stream()andemit_error_to_all_streams()in the same file already read error codes withto_u32()after anis_number()guard — this change matches them exactly. - Test wires session
error/closeon both ends to reject the awaited promise, covers server+client sessions and bothsend_go_awaycall sites (with/without opaqueData), and asserts exact code/lastStreamID/opaqueData.
Extended reasoning...
Overview
Single-line native fix in H2FrameParser::goaway() (src/runtime/api/bun/h2_frame_parser.rs): the GOAWAY error code argument is now read with JSValue::to_u32() instead of to_int32(), and the ErrorCode wrapper is constructed once at the read site rather than at each of the two send_go_away call sites. The old to_int32() saturates doubles above i32::MAX, so any code in (0x7fffffff, 0xffffffff] went out on the wire as 0x7fffffff. RFC 9113 §6.8 defines the field as unsigned 32-bit, and Node reads it with Uint32Value. A 40-line it.each test in node-http2.test.js sends 0xffffffff and 0x80000000 from both server and client sessions and asserts the peer's 'goaway' event reports the exact code, lastStreamID, and opaqueData.
Security risks
None. This widens the range of an integer that is written verbatim into a fixed 4-byte GOAWAY frame field. No allocation sizing, no indexing, no length arithmetic depends on it. The is_number() guard is preserved before the coercion, and to_u32() clamps rather than wraps, so out-of-range inputs cannot produce anything outside [0, u32::MAX].
Level of scrutiny
Low-to-medium. The behavioral change is one token (to_int32 → to_u32); the rest is moving the ErrorCode(...) construction up so both call sites take the value directly. I verified in src/jsc/JSValue.rs:784 that to_u32() is to_int64().clamp(0, u32::MAX as i64) as u32, which is the identity on the [0, 2^32-1] range that ServerHttp2Session#goaway already validates via validateInteger(code, "code", 0, kMaxUint32). I also confirmed the two sibling error-code reads in the same file — rst_stream() at line 7341+ and emit_error_to_all_streams() at line 8698 — use the identical is_number() guard + to_u32() pattern, so this change brings goaway() into consistency rather than introducing a new idiom.
Other factors
The test follows the repo's review rules well: it awaits a promise resolved from the 'goaway' event handler, wires sessionError/error/close on both sides to reject (so a lost frame fails with a cause instead of timing out), covers the variant matrix (server/client × with/without opaqueData × two boundary values), asserts an exact object with toEqual, and cleans up in finally. The PR body documents that all four cases fail on the released binary (receiving 2147483647) and pass with the fix, and that the rest of the 351-test file plus the ported Node goaway tests still pass. The description also explicitly addresses the out-of-validated-range behavior difference (to_u32 clamps, V8 wraps) and why it doesn't matter here — those inputs are rejected at the JS layer. No outstanding reviewer comments; no prior claude[bot] review on this PR.
|
Heads up from #37554: that PR no longer makes the client range-check the code. Review there pointed out that Node's What Node v26.3.0 puts on the wire for those (
|
node reads the code with Uint32Value, so a number outside 0..2^32-1 wraps: -1 goes out as 0xffffffff and 2^32 as 0. to_u32() clamps, so -1 went out as 0 (NO_ERROR) and 2^32 as 0xffffffff. session.destroy(error, code) goes through the same read. The test gets rows for -1, 2^32, 2^32 + 1 and Infinity from a client session, and compares the opaque data by bytes so that it also passes under node.
robobun
changed the title
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the new coerce_to_i32(...)? as u32 read at h2_frame_parser.rs:4711: JSC toInt32 is the modular ToInt32, so reinterpreting it as u32 is bit-identical to Node's Uint32Value for every double (NaN/Infinity -> 0, -1 -> 0xffffffff, 2^32+1 -> 1), and behind the preceding is_number() guard it cannot re-enter JS or throw. The sibling to_u32() (clamping) reads in rst_stream are unaffected because Http2Stream.close validates code as an integer before it reaches native. Note the PR description still describes the earlier to_u32() version (clamp: goaway(-1) -> 0), which no longer matches the code or the new test rows.
Extended reasoning...
The change replaces a saturating to_int32 read of the goaway() errorCode argument in src/runtime/api/bun/h2_frame_parser.rs with a wrapping ToInt32-then-u32 read and threads the single ErrorCode through both send_go_away call sites, plus an 8-row it.each matrix in test/js/node/http2/node-http2.test.js. It touches no auth, crypto, or injection surface; the only numeric boundary is handled by the modular conversion. Two confirmed findings are posted inline, so a human look is already signalled; this note only records what else was ruled out.
|
The read is ToUint32 now (021468d), which the comment above asked for. With What changed in this push:
Checked with a debug build and a raw TCP peer that reads the code bytes of the GOAWAY frame: 14 codes,
The description has the table. |
This branch has not been deployed
1 participant
Problem
session.goaway(0xffffffff)reaches the peer as0x7fffffff, like every code from 2^31 to 2^32 - 1. Node v26.3.0 sends0xffffffff.H2FrameParser::goawayreads the code withJSValue::to_int32()(h2_frame_parser.rs:4710), which saturates a double ati32::MAX.session.destroy(error, code)uses the same read.Fix
coerce_to_i32(global)? as u32: JavaScript's ToUint32, the conversion behind Node'sUint32Value.0xffffffffand 2^32 is 0.test/js/node/http2/node-http2.test.js, 8 cases. 7 fail on 1.4.3-canary, and all 8 pass under Node v26.3.0. Also ran the whole file and the ported goaway tests.Background
to_int32()saturates andto_u32()clamps.coerce_to_i32()calls JSC'stoInt32, which wraps.to_u32()and sentclient.goaway(-1)as 0. Main and Node send0xffffffff.Downsides
0x7fffffff. No such caller is known.goaway()makes 1 more native call, with 0 allocations. A session calls it a few times at most.ERR_OUT_OF_RANGEfor a code outside 0..2^32 - 1, which Node accepts. node:http2: validate goaway() arguments the same way on client and server sessions #37554 owns that check.Notes
Repro
Bun 1.4.3-canary and main print
7fffffff. Node v26.3.0 and this branch printffffffff.The error code on the wire
A raw TCP peer reads the 4 code bytes of the GOAWAY frame that
client.goaway(code)writes. One connection per cell.client.destroy(new Error("x"), code)gives the same value in every row, on all three.2**31 - 17fffffff7fffffff7fffffff2**31800000007fffffff800000002**32 - 1ffffffff7fffffffffffffff-1ffffffffffffffffffffffff-(2**31)8000000080000000800000002**32000000007fffffff000000002**32 + 1000000017fffffff00000001Infinity000000007fffffff00000000-Infinity000000008000000000000000NaN0000000000000000000000001.5000000010000000100000001-1.5ffffffffffffffffffffffff2**53000000007fffffff00000000A server session gives the Node value for every integer code from 0 to 2^32 - 1 in the table. For any other number its JS wrapper throws
ERR_OUT_OF_RANGEbefore the native read, on main and on this branch.Node's read:
node_http2.cc#L2992-L2995.Http2Session::Destroyreads its code the same way (#L2924-L2927). Node'sgoaway()in JS only runsvalidateNumber(code).Why not
to_u32()to_u32()isto_int64()clamped to 0..2^32 - 1. It is the identity inside that range. Outside it-1becomes 0 and2**32becomes0xffffffff. The first is worse than main, which sends0xfffffffffor-1because thei32value -1 is cast tou32.The test
'goaway'event reports the 4 code bytes of the frame, so the test reads the code there.0xffffffffand0x80000000from both session kinds, with and without opaque data (the two send paths of the native method). Rows 5 to 8 send-1,2**32,2**32 + 1andInfinityfrom a client session, which passes any number to the native method.undefinedand bun passes an empty Buffer (src/js/node/http2.ts:4311,:5322). That difference is older than this PR and is not changed here.BUN_JSC_validateExceptionChecks=1reports nothing for the new read.Not changed
lastStreamIDread a few lines below. It also differs from Node for 2^31 and above, and nghttp2 refuses an id of the sender's own parity. That is a separate change.rst_stream,emit_error_to_all_streams).Related
goaway()arguments in JS the same way on both session kinds. With it, any number reaches this read from a server session too. The two PRs merge without a textual conflict.goaway()sends nothing when the opaque data does not fit a frame) adds a check a few lines below in the same function. The two PRs merge without a textual conflict.Suites run with the debug build
test/js/node/http2/node-http2.test.js(401 pass, 6 skip), and fromtest/js/node/test/parallel:test-http2-goaway-opaquedata,test-http2-goaway-delayed-request,test-http2-server-shutdown-before-respond,test-http2-server-shutdown-options-errors,test-http2-server-shutdown-redundant,test-http2-session-graceful-close,test-http2-client-destroy,test-http2-session-cleanup-on-nghttp2-goaway.[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
root cause · written by the author bot
The native goaway() read the errorCode argument with a saturating to_int32 conversion, so codes at or above 2^31 (such as 0xffffffff) were clamped rather than wrapped and the wrong value went on the wire, unlike Node's ToUint32 read. The fix coerces the argument with the modular ToInt32 and reinterprets the result as u32, which is bit-identical to Node's Uint32Value for every double, and threads that single ErrorCode through both send_go_away call sites. A parameterized test covers client and server sessions, both opaqueData paths, and out-of-range inputs such as -1, 2^32+1 and Infinity.