Repository navigation
Conversation
The shim's stack lsxpack_header[128] buffer was an allocation convenience, not a policy limit, but the bounds check turned it into a hard outbound cap: any HEADERS block with more than 128 field lines (125 custom + 4 pseudo) made sendHeaders() return false, nothing went on the wire, and the idle timeout eventually tore down the whole connection cleanly with no error surfaced. application.maxHeaderPairs is an inbound decoder limit (node's DefaultApplication applies it in the header-set callback), so the encoder path must not cap the count. Heap-allocate the lsxpack_header array above the stack threshold; lsquic copies the encoded block before returning so the buffer can be freed immediately. Also: only mark wrote_to_lsquic in bind_raw once the replay actually reaches lsquic, matching the already-bound path.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 11:14 AM PT - Jul 25th, 2026
⏳ @robobun, your commit 2e3cb71 is still building in
Add |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
lsxpack_header_set_offset2 asserts val_offset <= LSXPACK_MAX_STRLEN (65535), live in debug builds. Rebasing hdr->buf to the start of each name keeps the stored offsets at 0 and name_len+1 regardless of how far into the joined buffer the entry sits, so a >64KB header block no longer trips the assert. Tighten the name_len guard to keep name_len+1 in range. Test: capture the server-side sendHeaders() return via a resolver and assert it from the test body rather than throwing inside the onheaders callback; wire stream.closed to the response resolver so a regression surfaces the real error instead of hanging. Lengthen request header values so the joined buffer runs past 64KB.
There was a problem hiding this comment.
Both earlier findings are addressed in c6cb882 — the per-entry buf + name_off rebase keeps val_offset = name_len + 1 within lsxpack_strlen_t, and the test now resolves serverSendOk from the callback and asserts in the body (with stream.closed.catch(gotHeaders.reject) wired). I didn't find anything new, but this is manual malloc/free on an H3 wire-encode path, so worth a human look.
What was reviewed:
- Heap spill:
free(hdrs)reached on every exit (earlygoto done,count != expected, and success);hdrs != stack_hdrscorrectly gates it. - Tightened
name_len >= LSXPACK_MAX_STRLENguard keeps the rebasedval_offset≤UINT16_MAX;val_len > LSXPACK_MAX_STRLENunchanged. bind_rawnow setswrote_to_lsquiconly aftersend_headers == 0, matching the already-bound path — checkedabort_for_destroy's defer branch still behaves.- Test's 200×400-byte values push the joined buffer past 64 KB and asserts first/last header values byte-for-byte at the peer.
Extended reasoning...
Overview
The PR removes the hard 128-header cap in us_nq_stream_send_headers (packages/bun-usockets/src/node_quic_shim.c) by heap-allocating the lsxpack_header[] array when expected > 128, and rebases each entry's buf pointer to buf + name_off so the u16 name_offset/val_offset fields never wrap regardless of total buffer size. It also adjusts bind_raw in src/runtime/node/quic/stream.rs to set wrote_to_lsquic only after the pending-headers replay actually succeeds, and adds a round-trip test with 204 field lines and an ~82 KB joined buffer.
Prior review
On the first revision (16a53f1) I flagged (🔴) that lifting the count cap without addressing the u16 offset fields would silently corrupt headers once the joined buffer crossed 64 KB, and (🟡) that the test asserted inside an event callback. Commit c6cb882 applies the per-entry rebase I suggested (with the name_len >= LSXPACK_MAX_STRLEN tightening needed to keep name_len + 1 in range), moves the server-side assertion into the test body via a resolver, and wires stream.closed to reject gotHeaders. The test now uses 400-byte values specifically to push the buffer past 64 KB and asserts the first and last received values byte-for-byte, so the offset-rebase path is actually exercised.
Security risks
The encode path takes user-controlled header data. The offset-wraparound risk (garbage bytes on the wire) is what the rebase fixes; individual name/value lengths are still bounded by LSXPACK_MAX_STRLEN. (size_t)expected * sizeof(*hdrs) widens before multiply and expected is a signed int derived from the JS pair count, so no realistic allocation-size overflow on 64-bit. The heap buffer is freed on every path via the done: label. I don't see a new injection or memory-safety exposure.
Level of scrutiny
Medium-high: this is native C doing manual allocation on an HTTP/3 wire-encode path in a Node-compat module. The change is small and localized, but the failure mode of getting it wrong is silent wire corruption or a leak. The bind_raw reordering also subtly changes when abort_for_destroy takes the defer-vs-reset branch for a stream whose pending-headers replay failed — that looks correct (matches the already-bound send_headers path) but is the kind of thing a maintainer familiar with the lsquic session lifecycle should confirm.
Other factors
The bug-hunting pass on this revision found nothing. The test is in the existing quic-stream.test.ts file, follows the sibling tests' resolver pattern, and covers both directions (client→server and server→client) plus both the count and >64 KB paths. Deferring rather than approving because native protocol-shim changes with manual memory management are outside the "simple/mechanical" bar even when they look right.
There was a problem hiding this comment.
Both earlier review points are addressed in c6cb882 — the per-entry buf + name_off rebase keeps val_offset = name_len+1 bounded regardless of joined-buffer size, and the server-side sendHeaders return is now asserted from the test body with stream.closed wired to reject. I didn't find anything further, but this is native C heap management on an H3 encode path so worth a human pass.
What was reviewed:
- Traced every exit from
us_nq_stream_send_headers(early returns, bothbreaks,goto done, success fall-through) — heaphdrsis freed on all of them;malloc(notcalloc) is fine sincelsxpack_header_set_offset2memsets the struct and the pre-PR stack array was also uninitialized. - Confirmed the stack-then-heap-spill shape mirrors the existing
us_quic_stream_send_headersinquic.c:985-1013. - Checked the
bind_rawchange:wrote_to_lsquicnow gates onsend_headers == 0, matching the already-bound path; only downstream reader isabort_for_destroy, where the new behavior (defer abort if nothing actually reached lsquic) is the more correct one.
Extended reasoning...
Overview
The PR removes a hard 128-entry cap in us_nq_stream_send_headers (packages/bun-usockets/src/node_quic_shim.c) by spilling the lsxpack_header[] array to the heap when expected > 128, keeping the stack buffer for the common case. After my earlier review it also rebases hdr->buf per entry so lsxpack offsets stay small independent of the joined buffer's total size, and tightens the name_len guard to >= so val_offset = name_len + 1 never exceeds LSXPACK_MAX_STRLEN. A small Rust change in bind_raw (src/runtime/node/quic/stream.rs) delays setting wrote_to_lsquic until the pending-headers replay actually succeeds, matching the direct send_headers path. The test round-trips 204 field lines (200 × 400-byte values, ~82KB joined) in both directions.
Prior feedback and its resolution
I left two inline comments on the first revision: (1) the offset-into-single-buffer scheme could exceed the setter's LSXPACK_MAX_STRLEN assert once the joined buffer grew past 64KB, and (2) an expect() inside the server onheaders callback would degrade a regression into a timeout. Both are addressed in c6cb882: the per-entry base pointer keeps offsets at 0 and name_len+1; the test captures the server return via serverSendOk.resolve(...), asserts it in the test body, and wires stream.closed.catch(gotHeaders.reject). The test's header values were bumped to 400 bytes so the joined buffer exceeds 64KB and exercises the rebase path.
Security risks
The function operates on a buffer built by Bun's own buildNgHeaderString (Rust side), so expected and the buffer contents are trusted-caller inputs, not raw wire bytes. expected is an int cast to size_t before the multiply, and sizeof(*hdrs) is small, so the allocation size can't wrap on 64-bit; a pathological count would just fail malloc and return -1. The per-entry length guard rejects individual names/values ≥ LSXPACK_MAX_STRLEN. No new externally-controlled parsing was introduced.
Level of scrutiny
This is manual C memory management (malloc/free with goto-based cleanup) on an HTTP/3 QPACK encode path across an FFI boundary — exactly the category REVIEW.md flags as most-blocked. The change is small, follows the existing pattern in quic.c, and I've walked every exit path for the free, but native protocol code with heap allocation deserves a maintainer's eyes rather than a bot approval.
Other factors
node:quic is experimental (the test output shows the ExperimentalWarning). The gate evidence shows the new test fails on main (sendHeaders returns false) and passes on the PR under both ASAN-debug and release. No CODEOWNERS file matched. No unresolved reviewer threads remain — both of my prior inline comments are marked resolved with responsive fix commits.
Diff is ready. The new test fails on main ( CI red on builds 80942 and 81075 is unrelated to this change:
Both review threads are resolved in c6cb882. |
|
Closing this PR because it has been inactive for more than 90 days. |
|
The stale sweep closed this PR. It was not merged. Edited after posting: the first version of this comment said that the bug is still present on Status today, on canary
Corrections to my earlier text on this PR
I am preparing a small PR from current |
Problem
Sending an HTTP/3 HEADERS block with more than 128 field lines via
node:quicsilently stalls the stream and eventually kills the whole connection:sendHeaders()returnsfalseand nothing goes on the wire. The stream never settles. Other streams on the session continue to work. If the session has no other traffic, the idle timeout closes it, andstream.closed/session.closedresolve without an error. Settingapplication.maxHeaderPairs: 65535has no effect on the outbound path.Corrected after the PR was closed: an earlier version of this paragraph said that every concurrent stream on the session dies. That was wrong.
Cause
us_nq_stream_send_headersinpackages/bun-usockets/src/node_quic_shim.cbuilds a stackstruct lsxpack_header[US_NQ_MAX_HEADERS]withUS_NQ_MAX_HEADERS = 128and bails with-1whenexpected > 128. That was never a policy limit, just the stack buffer size;application.maxHeaderPairsis an inbound decoder limit (applied innq_hsi_process_header) and node's encoder path imposes no count cap.With the shim returning
-1, the Rust caller returnsfalsefromsendHeaders,createBidirectionalStream({headers})ignores it, and the stream sits in lsquic withwant_read(true)but no bytes ever sent. Thefor awaitblocks untilmaxIdleTimeoutfires aCONNECTION_CLOSE(0).Fix
Heap-allocate the
lsxpack_header[]array whenexpected > 128; keep the stack buffer for the common case.lsquic_stream_send_headersencodes into its own buffer before returning, so the array is freed immediately after.Also: in
bind_raw, only setwrote_to_lsquiconce the pending-headers replay actually reaches lsquic (matches the already-bound path atsend_headers).Test
test/js/node/quic/quic-stream.test.tsround-trips a request and a response with 204 field lines each, assertingsendHeaders()returnstrueand every header arrives at the peer. Fails on main withexpect(stream.sendHeaders(req, { terminal: true })).toBe(true)receivingfalse.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file