Repository navigation
Conversation
get_padding() returned the aligned padding and every caller then added one more byte for the Pad Length octet. So PADDING_STRATEGY_ALIGNED frames were one byte past a multiple of 8. get_padding() now returns the Pad Length as an Option, sized like nghttp2's padded payload length. Some(0) is a PADDED frame that grows by the Pad Length octet alone. The callers pass the real payload bound. Queued DATA frames were capped at their own length and never padded. A HEADERS frame with priority fields left them out of the aligned length and wrote Pad Length after them. RFC 9113 6.2 puts Pad Length first.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
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 one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 15 minutes for your next included review. Limit 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
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 finding, I also checked two things in src/runtime/api/bun/h2_frame_parser.rs and ruled them out: the (padded_len - frame_len - 1) as u8 cast in get_padding (line ~1473) cannot underflow or truncate, since frame_len < max_payload_len guarantees padded_len >= frame_len + 1 and max_padded_len caps it at frame_len + 256; and write_padded's 16384-byte rare-data buffer (line ~4241) cannot be overrun, since both DATA callers now pass a max_payload_len already clamped to MAX_PAYLOAD_SIZE_WITHOUT_FRAME (16374).
Extended reasoning...
Findings were reported inline, so this body only records what else was examined. The two concerns above were traced through the new get_padding bounds and both write_padded call sites (flush_queue and the direct-send path at ~5212), where max_size is MAX_PAYLOAD_SIZE_WITHOUT_FRAME.min(...) before being handed to get_padding; the padded payload therefore never exceeds 16374 bytes and the u8 cast stays in range on every path.
nghttp2 bounds the padded length of a HEADERS frame by NGHTTP2_MAX_PAYLOADLEN (16384), not by the peer's SETTINGS_MAX_FRAME_SIZE. For a peer that allows larger frames, a header block near 16 KB was padded past 16384 bytes under PADDING_STRATEGY_MAX, and a larger block was padded at all. node does neither.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 3:37 PM PT - Sep 19th, 2026
✅ @robobun, your commit fd7e884b8a5dff4752a9d90449f067ea5a7bb60a passed in 🧪 To try this PR locally: bunx bun-pr 43554That installs a local version of the PR into your bun-43554 --bun |
There was a problem hiding this comment.
I re-reviewed after bc4951c and found no bugs; the 16384 HEADERS bound and its boundary test cover the point I raised earlier. Since this changes the bytes node:http2 puts on the wire in three separate paths (queued DATA, direct DATA, request HEADERS), a human look is still worthwhile before merge.
What was reviewed:
get_paddingarithmetic: withframe_len < max_payload_lenthe result is always in0..=255and the padded payload never exceedsmax_payload_len, sowrite_padded's 16384-byte scratch buffer (payload capped at 16374) and the flow-control window (padding is charged toremote_used_window_size) both hold.- The HEADERS path: a
Somepadding impliespayload_size <= actual_max_frame_size, so the CONTINUATION branch never sees a padded block andfirst_chunk_sizestays in range; Pad Length now precedes the priority fields per RFC 9113 6.2. flush_queue: droppingframe_remainingfrommax_sizeis what lets a whole queued frame pad into leftover window room; the chunk branch sends exactlymax_sizebytes so it correctly no longer pads.- I could not run the new tests here (no debug build in this checkout), so the test expectations are checked by reading against the code, not by execution.
Extended reasoning...
Overview
The PR reworks outbound HTTP/2 frame padding in /home/claude/bun/src/runtime/api/bun/h2_frame_parser.rs so that the Pad Length octet counts toward the padded payload length, matching nghttp2's select_padding contract. Stream::get_padding now returns Option<u8>, a padding_overhead helper replaces three inline computations, flush_queue no longer caps the padded size at the frame's own length, the direct DATA path passes max_size instead of max_size - 1, and the HEADERS path in request() writes Pad Length before the priority fields and pads from a static zero array instead of shifting the encoded header block in place. The second commit adds MAX_PADDED_HEADERS_PAYLOAD (16384) as an upper bound on padded HEADERS regardless of the peer's SETTINGS_MAX_FRAME_SIZE, which addresses the inline comment from the previous review round. The test file gains a ~330-line describe block with raw-socket frame captures pinned to node v26.3.0 output.
Security risks
None of consequence. Padding bytes come from a static zero array or a zero-filled scratch buffer, so no uninitialized memory reaches the wire. All lengths are derived from local state (window sizes, local encoded size) rather than untrusted peer input, and get_padding returns early when frame_len >= max_payload_len, so the padded_len - frame_len - 1 subtraction cannot underflow and the as u8 cast cannot truncate. The removed try_reserve guard is no longer needed because nothing allocates on that path anymore; the remaining .expect("int cast") on payload_size is bounded by the frame size setting (at most 2^24-1) and was pre-existing.
Level of scrutiny
Moderate-to-high. This is protocol framing code where an off-by-one changes what every peer sees, and the change spans three independent write paths plus a helper semantics change (0 previously meant "unpadded"; now Some(0) is a legitimately padded frame). I traced each call site: the direct DATA path reproduces the old MAX sizes exactly (the tests confirm the ninth case passes on the unfixed build) and fixes the one-byte overshoot for ALIGNED; the queued path now pads into leftover window room and charges padding to both stream and connection windows before writing; the HEADERS path preserves the CONTINUATION decision at the exact-fit boundary (encoded_size + priority_overhead == actual_max_frame_size still takes the single-frame branch as before). The Some(pad_length @ 1..) pattern is already used elsewhere in the tree on the pinned nightly.
Other factors
The bug-hunting pass ran dry with no findings, and the previous round's only inline concern is resolved by code and a dedicated boundary test in bc4951c. No CODEOWNERS entry covers the changed paths. I did not execute the new tests because no debug build exists in this checkout and a from-scratch build is not feasible in a review run; the PR description claims the full http2 suite and node's 261 test-http2-* files pass, which I could not independently confirm. Given the wire-visible nature of the change and the untested-here status, I am not comfortable asserting that no human needs to look, hence defer rather than approve.
There was a problem hiding this comment.
This automated review re-ran on the latest push and found no bugs; the HEADERS 16384-byte cap raised last time is now in place (MAX_PADDED_HEADERS_PAYLOAD). Since this changes the bytes node:http2 puts on the wire across four serialization paths, a human look is still worthwhile before merging.
What was reviewed:
get_paddingarithmetic:padded_len - frame_len - 1cannot underflow (theframe_len >= max_payload_lenearly return plus theframe_len + 1 + 255cap keeppadded_lenin[frame_len + 1, frame_len + 256]), so theas u8is exact.- Flow control:
flush_queueandsend_datanow pass the full window asmax_payload_len, and the padded payload (Pad Length octet included) never exceeds it; thewrite_paddedscratch buffer (16384) covers the largest padded DATA payload (16374). request()single-frame decision: a padded payload is bounded bymin(peer max_frame_size, 16384)so it always fits one HEADERS frame; the CONTINUATION branch is unchanged and only reached unpadded.- Traced the queued-frame test cases (windows 3/5/6/1000) through the new branches by hand; expected lengths and Pad Length values match the code.
Extended reasoning...
Overview
The PR reworks outbound HTTP/2 frame padding in src/runtime/api/bun/h2_frame_parser.rs: Stream::get_padding now returns Option<u8> with nghttp2 select_padding semantics (the Pad Length octet counts toward the padded length, Some(0) is a PADDED frame with no trailing bytes), a padding_overhead helper replaces four copies of the same inline arithmetic, flush_queue bounds the payload by flow control and the frame-size limit instead of the queued frame's own length, the chunk branch of flush_queue no longer pads (the chunk already fills the window), and request() writes Pad Length before the priority fields and streams the padding from a static zero array instead of growing and shifting the encoded header block. The test file adds nine raw-socket tests in test/js/node/http2/h2-conformance.test.ts that pin frame lengths and Pad Length values, for both DATA and HEADERS, ALIGNED and MAX, direct and queued paths, and the 16384 HEADERS cap against a peer that advertises a larger SETTINGS_MAX_FRAME_SIZE.
Security risks
Padding is purely outbound and computed from local sizes and the peer's advertised window / frame size, both of which are already clamped elsewhere. I checked the two casts the reviewer notes flagged: (padded_len - frame_len - 1) as u8 is provably in range given the early return and the + 1 + 255 cap, and u32::try_from(payload_size).expect(...) operates on values at most 16384. The write_padded scratch buffer is a fixed 16384-byte array and the largest padded DATA payload is MAX_PAYLOAD_SIZE_WITHOUT_FRAME (16374), so there is no out-of-bounds write. The batch.resize in the batched send_data path adds exactly pad_length zeros. No parsing of untrusted input changes here.
Level of scrutiny
Moderate to high. This is wire-format serialization in node:http2, so a mistake would show up as peer-side FRAME_SIZE_ERROR or flow-control violations rather than a crash, and it would only affect users who opt into a non-default paddingStrategy (none of node's 261 test-http2-* files set one). The new tests assert exact byte layouts and were reported by the author to fail on the previous release, which is the right shape of evidence. The removal of padding on the flush_queue chunk branch is a behaviour change, but it matches nghttp2 (frame->hd.length >= max_payloadlen means no padding) and it fixes a real window overrun in the old code. Still, four separate serialization sites changed and the arithmetic is easy to get off by one, which is why a human pass is worth it rather than an outright approval.
Other factors
My prior inline finding (HEADERS padding must be capped at 16384 regardless of the peer's SETTINGS_MAX_FRAME_SIZE) was addressed in a follow-up commit with MAX_PADDED_HEADERS_PAYLOAD and a dedicated test; the thread was resolved by this bot's own session, but the code change is verifiable in the diff. The github-actions[bot] inline comments that followed all point at comment lines, and a later commit tidied those comments, so nothing there reads as an outstanding human objection. No CODEOWNERS entry covers the changed files. The bug hunt exited on a dry streak rather than a budget bound.
|
Overlap note. #43544 now removes the PRIORITY field from the HEADERS writer in |
|
A review on #43599 asked that bun match node v26.3.0, which never sets the PRIORITY flag on a HEADERS frame (nghttp2 1.69.0 discards the priority spec): #43599 (comment) #43599 now removes the priority fields from the native HEADERS writer. That overlaps with this PR in two places:
The padding length fix itself ( |
Problem
PADDING_STRATEGY_ALIGNED, every padded HEADERS and DATA frame is one byte too long: 17 bytes, node v26.3.0 sends 16.Stream::get_padding(src/runtime/api/bun/h2_frame_parser.rs:1445) returned the padding that aligns the payload. Each caller added the Pad Length octet. Zero meant "not padded", so a frame one byte short had no answer.flush_queuecapped the padded size at the frame's own length, so queued DATA frames got no padding.request()aligned without the priority fields and wrote Pad Length after them. RFC 9113 6.2 puts it first.Fix
get_paddingreturnsOption<u8>: the Pad Length, orNonefor no padding. As in nghttp2, the Pad Length octet counts toward the padded length.Some(0)is a PADDED frame that grows by that octet alone.flush_queuebounds the payload by flow control and the frame size.request()aligns the whole payload, writes Pad Length first, and stops padding at 16384 bytes like nghttp2.test/js/node/http2/h2-conformance.test.ts: 12 new tests, 11 fail on bun 1.4.3. Alsotest/js/node/http2/and node's 261test-http2-*.jsfiles.Background
ALIGNEDmakes frame header plus payload a multiple of 8.OnDWordAlignedPaddingreturns the padded payload length. nghttp2 sets Pad Length to that length minus the data minus one.send_data, or from a queue inStream::flush_queuewhen flow control or backpressure blocks.Notes
Repro: an
http2.createServer({ paddingStrategy: PADDING_STRATEGY_ALIGNED })that answersstream.respond({ ":status": 200 }, { sendDate: false }); stream.end("hello"). A raw socket client prints the frames of stream 1.Probes where this branch and node v26.3.0 now print the same frames:
ALIGNEDandMAX, response bodies of 1 to 9, 14 to 16, 250 and 255 to 257 bytes. A body of 6 bytes giveslength=7, Pad Length 0. A body of 7 bytes gives an unpadded frame.SETTINGS_INITIAL_WINDOW_SIZE=0and then sends WINDOW_UPDATE, so the body goes through the queue. For a 5-byte body: window 1000 giveslength=7 pad=1(ALIGNED) andlength=261 pad=255(MAX). Window 100 giveslength=100 pad=94(MAX). Window 6 giveslength=6 pad=0. Window 5 gives an unpadded frame. Window 3 then 1000 gives an unpadded 3-byte frame, thenlength=7 pad=4. bun 1.4.3 sends every one of these unpadded.test/parallel/test-http2-padding-aligned.jsexpects 74 bytes from the client and 98 from the server. bun 1.4.3 writes 75 and 100. This branch writes 74 and 98. That test still cannot run in bun as it is, because it also asserts the size of each socket write and bun writes the same bytes in fewer chunks.PADDED + PRIORITY HEADERS, a bun client with
request(headers, { exclusive: true })against a raw server, payload in hex:Closed PR #32995 had the same layout fix. It was closed as stale, not rejected. #41520 states that the layout fix stays separate from it. node v26.3.0 no longer sets the PRIORITY flag at all, so RFC 9113 6.2 is the reference for this frame.
flush_queuedetail: the chunk branch (the window is smaller than the frame) sends exactlymax_sizebytes, so padding can never fit there. nghttp2 does the same (frame->hd.length >= max_payloadlenmeans no padding). That branch no longer callsget_padding.request()detail: the Pad Length octet, the priority fields, the header block and the padding now go to the writer as separate writes. The padding comes from a static zero array. The old code grew the header blockVecand moved the block by one byte, which needed an allocation check before the frame header went out. That check and itsFailed to allocate padding buffererror are gone, because nothing allocates any more.Found while working on this, not fixed here, and not tracked anywhere yet:
ALIGNED: PUSH_PROMISElength=23 pad=1, the empty END_STREAM DATA framelength=7 pad=6, trailerslength=15 pad=2). Those frames never reachget_padding:send_trailers,push_promise, and the zero-length branches ofsend_dataandStream::flush_queuehave no padding logic.client.request(headers, { parent: 0 })fails withNGHTTP2_INTERNAL_ERROR, and the next request on the session dies with GOAWAYCOMPRESSION_ERROR. node v26.3.0 answers both with 200. The check isparent <= 0inrequest(), and it runs after the header block went into the shared HPACK encoder. Closed node:http2: fix request() option validation ordering, parent: 0, and the PADDED+PRIORITY HEADERS layout #32995 had the fix. node:http2: encode each header block in one call, all fields or none #41520 moves the option checks before the encode and leavesparent: 0out.DATA frames split at 16374 bytes in bun and at 16384 in node. That limit is not part of this change, so the sizes at which a large body stops being padded differ by 10 bytes.
One of the 12 new tests (
PADDING_STRATEGY_MAX keeps padded DATA frames inside the flow-control window) passes on bun 1.4.3 too.MAXsizes on the direct write path were already correct. The test pins them, because the same lines changed.A response of three 5-byte writes, one after the other, to a peer with a small stream window and no WINDOW_UPDATE. The lengths match node v26.3.0:
ALIGNEDwith a window of 20 gives7 (pad 1), 7 (pad 1), 6 (pad 0).MAXwith a window of 600 gives261, 261, 78 (pad 72). bun 1.4.3 underALIGNEDsends8, 8and then an unpadded 4-byte piece.HEADERS and a peer that allows large frames. nghttp2 bounds the padded length of a HEADERS frame by
NGHTTP2_MAX_PAYLOADLEN(16384), not by the peer'sSETTINGS_MAX_FRAME_SIZE(session_headers_add_pad). A review comment on this PR found thatrequest()used the peer's setting. With a raw server that advertises a max frame size of 32768, aMAXclient and a 16200-byte header block:For a block of 16420 bytes node sends no padding (it also splits the block at 16384 into HEADERS plus CONTINUATION). The first commit of this PR padded it. It now goes out unpadded in one frame, because the peer allows 32768.
Code comments are one line each. The links to the node and nghttp2 sources are in this description.
Self-review, the two rejected concerns:
get_paddingclamp that the DATA tests pin with windows of 6 and 5 bytes, and theMAXheader sweep innode-http2-continuation.test.tssends header blocks of 16128 to 16400 bytes through it.min()over the two windows.Suites run on the debug build of the final commit: all of
test/js/node/http2/*.test.*, and everytest/js/node/test/{parallel,sequential}/test-http2-*.js(261 files, all pass). None of those 261 files sets a padding strategy, so they cover the default path, which sends the same bytes as before.[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