Repository navigation
Conversation
…request header does not abort the connection us_quic_hsi_prepare (the server-side QPACK prepare_decode hook for Bun.serve http3) rejected any per-header decode request above 64 KB by returning NULL, which lsquic treats as a QPACK decompression failure and aborts the whole connection (HEC_QPACK_DECOMPRESSION_FAILED, app error 0x200). With Huffman the decoder reserves ~1.5x the encoded length, so a single header value of ~44 KB encoded (~59 KB decoded) was enough to take down every in-flight stream on the session. Two changes: * Build lshpack/lsqpack with -DLSXPACK_MAX_STRLEN=UINT32_MAX so lsxpack_strlen_t is 32-bit. This is struct-layout ABI shared by every consumer of lsxpack_header.h (lshpack, lsqpack, lsquic, quic.c, node_quic_shim.c, c-bindings.cpp), so it lives in globalFlags. Same change as #35741 for node:quic. * Cap us_quic_hsi_prepare at LSXPACK_MAX_STRLEN instead of 64 KB, matching nq_hsi_prepare_decode. lsqpack itself bounds prepare_decode requests at LSXPACK_MAX_STRLEN, and NULL from this hook is connection-fatal, so the only rejection we keep is the type's structural limit. es_max_header_list_size (16 KB on the server) is advisory only in lsquic: it is written into the SETTINGS frame but never enforced on decode, so it was not the guard the surrounding comment implied. Test added to serve-http3.test.ts sends a 60 KB x-big header, asserts the handler receives it intact and the pooled session carries a second request.
WalkthroughChangesQUIC header decode protection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with: bun test test/js/bun/http/serve-http3.test.ts -t "single large request header value"Before: The CI status: the diff is green on every lane that actually ran. The gate passed (test fails without the fix and passes with it on ASAN and release). All 40 debian/ubuntu test shards on build 81475 passed, and every |
|
Updated 9:14 PM PT - Jul 25th, 2026
❌ @robobun, your commit a17a875 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35763That installs a local version of the PR into your bun-35763 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35741. Same root cause, different code path:
The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked us_quic_hsi_prepare for unsigned overflow in need = h->len + space and an infinite loop in the ncap *= 2 doubling now that the cap is UINT32_MAX — ls-qpack's 24-bit varint bound keeps space ≤ ~25 MB in practice, so ncap tops out at 32 MB well before wrapping. The HTTP/2 (lshpack) decode path was also checked for a matching cap that this ABI change would need lifted — there isn't one.
Extended reasoning...
The bug-hunting system's DoS finding is the load-bearing concern here and warrants human review; the h3blast ABI mismatch is a real but dev-tool-only nit. Separately, three finder agents raised potential unsigned-overflow / infinite-loop issues in the same us_quic_hsi_prepare function after the cap moved to UINT32_MAX: h->len + (unsigned int) space wrapping, and while (ncap < need) ncap *= 2 never terminating if need were near UINT32_MAX. Verifiers traced ls-qpack's lsqpack_dec_int24 (caps value length at 1u << 24) and guarantee_out_bytes (requests ~1.5× that), so space is bounded around 25 MB in practice and the doubling loop reaches 32 MB without overflow — recording that here so it isn't re-derived.
…K hunks with #35741 Review found that lifting the prepare_decode cap to LSXPACK_MAX_STRLEN (UINT32_MAX after the build-flag change) removed the only server-side bound on peer-declared pre-allocation: lsqpack requests ~1.5x the declared value length before any value bytes arrive, so a handful of wire bytes could reserve ~32 MB per stream. Keep a hard 128 KB ceiling on the total hset buffer instead; that admits the 60 KB repro (and any single header up to ~85 KB) while bounding the worst case at 128 KB x es_init_max_streams_bidi per connection. Also: * Fix the stale comment above es_max_header_list_size (it is advisory only; the allocation guard lives in us_quic_hsi_prepare). * Add -DLSXPACK_MAX_STRLEN=UINT32_MAX to packages/h3blast/Makefile so its hand-rolled compile agrees with the dep objects it links. * Make the flags.ts and Makefile hunks byte-identical to #35741 so a 3-way merge resolves to one copy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/serve-http3.test.ts`:
- Around line 531-542: The HTTP/3 tests only cover successful large headers and
must also verify the rejection boundary. Extend the test coverage around fetchH3
to send a header value just over US_QUIC_HSET_MAX_BUF (128 KiB) and assert the
expected protocol failure, preserving the existing successful 60,000-byte and
follow-up request checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 431a5883-7e1c-4135-b2b9-caee661f26b6
📒 Files selected for processing (4)
packages/bun-usockets/src/quic.cpackages/h3blast/Makefilescripts/build/flags.tstest/js/bun/http/serve-http3.test.ts
There was a problem hiding this comment.
d7ab1ec addresses both earlier findings — us_quic_hsi_prepare now bounds the hset buffer at US_QUIC_HSET_MAX_BUF (128 KB) via h->len + space, and packages/h3blast/Makefile sets -DLSXPACK_MAX_STRLEN=UINT32_MAX so its layout matches the linked dep objects. The remaining inline note is the sibling node:quic decode hook, which the same flags.ts hunk also unbounds.
Extended reasoning...
The author pushed d7ab1ec after my previous review. I re-checked packages/bun-usockets/src/quic.c: the new US_QUIC_HSET_MAX_BUF guard ((size_t) h->len + space > 128*1024) sits before the realloc, so the earlier ~32 MB-per-stream amplification on Bun.serve({http3:true}) is closed. packages/h3blast/Makefile now carries the matching -DLSXPACK_MAX_STRLEN define. Both prior comments are resolved. The one open item (posted inline) is that the global build flag also flows to node_quic_shim.c's nq_hsi_prepare_decode, whose only pre-realloc guard is still space > LSXPACK_MAX_STRLEN; d7ab1ec did not touch that file. Not approving given this is a network-facing allocation bound.
The -DLSXPACK_MAX_STRLEN=UINT32_MAX build flag turns the sole guard in nq_hsi_prepare_decode (space > LSXPACK_MAX_STRLEN) into a 4 GB no-op, opening the same peer-declared-varint amplification on node:quic that the previous commit bounded for Bun.serve. Cap the decode buffer at twice the configured maxHeaderLength (Huffman headroom for a single header that is itself the whole budget), falling back to 128 KB when maxHeaderLength is unlimited. Verified against #35741's large-header test (maxHeaderLength 1 MB, 60 KB value).
With the default maxHeaderLength of 16384, 2x that is 32 KB, which is below the pre-PR threshold (space > UINT16_MAX). A header in the 32-64 KB decode-window range used to decode and get silently dropped by process_header (session stays alive); it would now be connection-fatal. Floor the cap at US_NQ_HSET_MAX_BUF so no configuration regresses; users who opt into a larger maxHeaderLength still get proportionally more.
There was a problem hiding this comment.
All three rounds of feedback are addressed in 907cc02 — the quic.c hset buffer is capped at 128 KB, nq_hsi_prepare_decode is capped at max(2 × maxHeaderLength, 128 KB) so the default config no longer regresses, and the h3blast Makefile carries the matching -DLSXPACK_MAX_STRLEN. I didn't find anything further, but this changes peer-controlled allocation bounds on a network-facing path and flips a global struct-layout ABI flag, so it's worth a human look — particularly the 128 KB cap value and the connection-fatal-vs-stream-reset deferral.
What was reviewed:
- Traced every TU that includes
lsxpack_header.h(lshpack/lsqpack/lsquic viacomputeDepFlags,quic.c,node_quic_shim.c,c-bindings.cpp,h3blast) — all get the flag;c-bindings.cpp's HTTP/2 path keeps its own local 64 KBLSHPACK_MAX_HEADER_SIZEcap and its Rust FFI struct usessize_t, so no ABI break there. - Re-checked the amplification bound after 907cc02:
quic.cworst case ≈ 128 KB × 100 streams ≈ 12.8 MB/conn;node:quicdefault ≈ 128 KB/stream (was effectively 64 KB before, so no regression). - Confirmed the
flags.tsentry has nowhenpredicate and flows throughcomputeDepFlags, so vendored deps and bun's own TUs agree on the 32-bit layout on every platform/profile.
Extended reasoning...
Overview
The PR lifts the 64 KB per-header QPACK decode-buffer cap in Bun.serve({http3: true}) so a ~60 KB request header no longer aborts the whole connection with H3_QPACK_DECOMPRESSION_FAILED. It does so by (1) building every lsxpack_header.h consumer with -DLSXPACK_MAX_STRLEN=UINT32_MAX (widening lsxpack_strlen_t from u16 → u32, a struct-layout ABI change), (2) replacing quic.c's hardcoded 64 KB per-request cap with a 128 KB whole-buffer cap, (3) adding a matching max(2 × maxHeaderLength, 128 KB) cap to the sibling node:quic hook, and (4) adding a regression test that sends a 60 KB header and asserts the pooled session survives.
Over three review rounds I flagged: the initial uncapped variant opened a ~25 MB/stream × 100-stream amplification DoS (fixed in d7ab1ec with the 128 KB cap); the h3blast dev tool's Makefile didn't set the flag and would link mismatched-layout objects (fixed in d7ab1ec); the same DoS on the node:quic sibling (fixed in de97474); and de97474's 2 × maxHeaderLength cap regressed the default config's abort threshold from 64 KB to 32 KB (fixed in 907cc02 with a 128 KB floor). All resolved.
Security risks
The change directly governs how much memory a peer-declared QPACK length varint can force the server to allocate before any value bytes arrive. The final state bounds this at 128 KB/stream for Bun.serve (≈12.8 MB/connection at the default 100 bidi streams) and max(2 × maxHeaderLength, 128 KB)/stream for node:quic. Exceeding the cap is still connection-fatal (NULL → LQRHS_ERROR → HEC_QPACK_DECOMPRESSION_FAILED), which is a documented follow-up (needs an lsquic patch to make it per-stream per RFC 9114 §4.2.2). The struct-ABI change is the other risk surface: any TU that includes lsxpack_header.h without the flag would read/write misaligned fields. I traced every consumer — vendored lshpack/lsqpack/lsquic get it via computeDepFlags → globalFlags (no when predicate), bun's own quic.c/node_quic_shim.c/c-bindings.cpp via computeFlags, and h3blast via its patched Makefile. The HTTP/2 HPACK path in c-bindings.cpp keeps its own local LSHPACK_MAX_HEADER_SIZE = 65536 shared-buffer cap and its Rust FFI struct (lshpack.rs) uses usize for lengths, so it's unaffected by the width change.
Level of scrutiny
High. This is production network-facing code that bounds peer-controlled allocation, and the fix required three iterations to get the DoS bound and the default-config behavior right. The 128 KB constant, the 2 × maxHeaderLength formula, the decision to keep over-cap connection-fatal for now, and the coordination with #35741 (byte-identical flags.ts/h3blast hunks) are all judgment calls a maintainer should sign off on.
Other factors
The regression test is well-constructed (asserts full value received + pooled-session survival, uses Buffer.alloc per harness convention, comment explains the Huffman/1.5× arithmetic). The PR body's evidence shows the test failing on main (ASAN lane) and passing on the PR, and reports serve-http3.test.ts, node/quic/*, and h2-conformance.test.ts all passing under the new layout. The CodeRabbit suggestion to test the rejection boundary was reasonably declined (would lock in a transient internal threshold and connection-fatal behavior that's slated to change).
There was a problem hiding this comment.
907cc02 addresses the last of my four findings (the nq_hsi_prepare_decode cap is now floored at 128 KB, so the default maxHeaderLength = 16384 no longer regresses below the pre-PR 64 KB abort threshold). I found nothing further in this revision, but this changes a DoS bound on network-facing header decode and flips a struct-layout ABI flag across lshpack/lsqpack/lsquic, so I'd like a human to sign off.
What was reviewed:
us_quic_hsi_prepare:h->len + spacebounded at 128 KB;(unsigned int)cast andncapdoubling stay well under overflow.nq_hsi_prepare_decode:(size_t) h->max_bytes * 2widens before multiply;max_bytes == 0and the 16 KB default both floor to 128 KB.- ABI reach of
-DLSXPACK_MAX_STRLEN=UINT32_MAX:c-bindings.cpp(HTTP/2) andsrc/http/lshpack.rsgo through thelshpack_headerwrapper withsize_tfields, notlsxpack_header_tdirectly, so the Rust↔C++ boundary is unaffected;h3blastnow matches.
Extended reasoning...
Overview
This PR lifts the 64 KB per-header QPACK decode-buffer cap in us_quic_hsi_prepare (Bun.serve HTTP/3) so a ~60 KB request header value no longer aborts the whole connection with H3_QPACK_DECOMPRESSION_FAILED. To do that it flips LSXPACK_MAX_STRLEN from the upstream UINT16_MAX default to UINT32_MAX globally (via scripts/build/flags.ts), which changes lsxpack_strlen_t from uint16_t to uint32_t in every TU that includes lsxpack_header.h — lshpack, lsqpack, lsquic, quic.c, node_quic_shim.c, c-bindings.cpp, and the h3blast dev harness. It then re-bounds the two prepare_decode hooks (128 KB total in quic.c; max(2 × maxHeaderLength, 128 KB) in node_quic_shim.c) so the widened type doesn't open a peer-controlled pre-allocation DoS. A new test in serve-http3.test.ts sends a 60 KB x-big header and asserts the handler receives it and the pooled session survives a follow-up request.
Security risks
The change sits directly on a DoS boundary: lsqpack requests the decode buffer from the peer's value-length varint before any value bytes arrive, so the cap is the only thing bounding how much heap ~13 wire bytes can reserve per stream. My first three review rounds on this PR each found a way the intermediate revisions widened that surface (unbounded quic.c cap → 32 MB/stream; unbounded node_quic_shim.c sibling; then a regression where the default maxHeaderLength halved the abort threshold). All are addressed in 907cc02: quic.c bounds the whole hset at 128 KB (~12.8 MB/conn worst case at 100 bidi streams), and node_quic_shim.c floors at 128 KB so no configuration aborts sooner than before. Exceeding the cap remains connection-fatal (an lsquic-level limitation the PR body tracks as follow-up). I also checked the reach of the struct-ABI change into HTTP/2: src/http/lshpack.rs mirrors the C++ wrapper struct lshpack_header (whose length fields are size_t), not lsxpack_header_t itself, so the Rust FFI layout is unaffected; and c-bindings.cpp's LSHPACK_MAX_HEADER_SIZE = 65536 passed to lsxpack_header_prepare_decode now fits without truncation.
Level of scrutiny
High. This is production network-facing server code (Bun.serve({http3: true}) and node:quic), it changes an allocation bound on adversarial input, and it flips a build flag that alters struct layout across three vendored libraries plus Bun's own HTTP/2 path. The PR body explicitly calls the shared flags.ts/h3blast hunks byte-identical to #35741, so merge order matters. The 128 KB cap is a judgment call (well-argued in the PR body, but still a policy choice about the connection-fatal threshold) and there's an acknowledged follow-up to make over-limit a per-stream reset instead.
Other factors
The gate evidence shows the new test fails on main (ASAN lane) and passes with the fix; test/js/node/http2/h2-conformance.test.ts and test/js/node/quic/* were run against the new layout. The bug hunting system found nothing in the current revision. Given the security-sensitive surface and the cross-library ABI change, I'm deferring rather than approving — a maintainer should confirm the 128 KB bound and the interim connection-fatal semantics are acceptable, and that landing order vs #35741 is understood.
|
Rebase |
Same shape as before (twice maxHeaderLength), with a 128 KB floor in place of 64 KB. The hunk is now identical to the one in #35763, so the second of the two to land rebases without a conflict here.
|
Heads-up from #35741, which is now rebased on f04caca. Main has a new consumer of
#35741 gives that file its own |
Same shape as before (twice maxHeaderLength), with a 128 KB floor in place of 64 KB. The hunk is now identical to the one in #35763, so the second of the two to land rebases without a conflict here.
This branch has not been deployed
2 participants
What
A single request header whose decoded value is about 59 KB (about 44 KB of encoded QPACK) aborts the entire
Bun.serve({http3: true})connection withH3_QPACK_DECOMPRESSION_FAILED(app error 0x200), taking every other in-flight stream on the session down with it.Cause
us_quic_hsi_prepare(packages/bun-usockets/src/quic.c, the server-side QPACK prepare_decode hook) rejected any per-header decode request above64 * 1024bytes by returningNULL. lsquic maps that toLQRHS_ERRORand callsci_abort_error(HEC_QPACK_DECOMPRESSION_FAILED), which is connection-fatal.With a Huffman-encoded value lsqpack reserves about 1.5x the encoded length, so the practical ceiling was about
65536 / 1.5 ≈ 43.7 KBof encoded bytes. A 60 KB value of'A'(6 bits/char Huffman) encodes to about 45 KB and requests about 67.5 KB of decode buffer, tripping the cap.es_max_header_list_size = 16 * 1024(set on the server engine) is advisory only: lsquic writes it into the SETTINGS frame but never enforces it on decode, so it was not the guard the surrounding comment implied.Fix
-DLSXPACK_MAX_STRLEN=UINT32_MAXsolsxpack_strlen_tis 32-bit in every TU that includeslsxpack_header.h(lshpack, lsqpack, lsquic,quic.c,node_quic_shim.c,c-bindings.cpp, and theh3blastdev harness). Theflags.tsandpackages/h3blast/Makefilehunks are byte-identical to node:quic(h3): build lshpack/lsqpack with 32-bit header lengths so large request headers don't abort the connection #35741 (thenode:quicside of the same root cause), so whichever lands first the other resolves cleanly.spacecap inus_quic_hsi_preparewith a 128 KB cap on the whole decoded header buffer (h->len + space). 128 KB admits a single header value up to about 85 KB (the 60 KB repro fits with room to spare) and bounds the per-stream buffer at 128 KB regardless of how many headers the peer sends. TheLSXPACK_MAX_STRLENcheck alongside it keeps the cast tolsxpack_strlen_tsound.nq_hsi_prepare_decodeinnode_quic_shim.c, since the build-flag change would otherwise turn its sole guard (space > LSXPACK_MAX_STRLEN) into a 4 GB no-op. The cap there ismax(2 × maxHeaderLength, 128 KB): users who opt into a largermaxHeaderLengthget proportionally more decode headroom, and the 128 KB floor means a small or default limit never lowers the abort threshold below where it was (a header overmaxHeaderLengthis still silently dropped inprocess_header, which keeps the session alive).es_max_header_list_sizeto say it is advisory and point at the actual guard.Why this cap, not
LSXPACK_MAX_STRLENlsqpack requests the decode buffer from the peer-declared value-length varint before a single value byte has arrived (
DATA_STATE_READ_VAL_LEN→guarantee_out_bytes), and that varint is 24-bit. Without a hard cap a dozen wire bytes reserve about 32 MB per stream, and withes_init_max_streams_bidi = 100one connection can pin about 3 GB of server heap for about 1 KB on the wire. The 128 KB cap keeps theBun.serveworst case at about 12.8 MB per connection.Exceeding the cap is still connection-fatal (same as before, at a higher threshold). Making it a per-stream reset per RFC 9114 §4.2.2 needs an lsquic patch:
NULLfromhsi_prepare_decodeand any nonzero fromhsi_process_headerboth end up inci_abort_errorviaqdh_header_read_results/qdh_hsi_process_wrapper/lsquic_stream.c'sHQFI_FLAG_ERRORpath. That and wiring a per-servermaxHeaderSizethrough to theBun.servehook are tracked alongside thenode:quicfollow-ups in #35741.Verification
New case in
test/js/bun/http/serve-http3.test.tssends a 60 KBx-bigheader, asserts the handler receives the full value, and asserts a second request on the pooled session still succeeds.Before: fetch rejects with
HTTP3StreamReset(server never invokes the handler).After: status 200,
x-big.length === 60000, follow-up request served.test/js/bun/http/serve-http3.test.ts,test/js/node/quic/*, andtest/js/node/http2/h2-conformance.test.tsall pass with the new struct layout. #35741's newquic-stream.test.tslarge-header case (maxHeaderLength: 1 << 20, 60 KB value) also passes under thenq_hsi_prepare_decodecap.[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file