Repository navigation
Conversation
…4 KB lsquic's send_headers_ietf encoded the header block into a fixed 64 KB stack buffer and failed with QWH_ENOBUF for a larger block, so the response never went out. A new lsquic patch sizes the buffer from the header list and uses the heap above 64 KB. The lsxpack offsets are 16-bit. The send and decode paths in quic.c and the send path in node_quic_shim.c addressed every field from the start of one shared buffer, so more than 64 KB of fields tripped the offset assertion in debug builds. Each field now points at its own slice of the buffer. The send-side hunks are the same as in #42895.
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughHTTP/3 header offsets now use per-entry storage. lsquic sizes encoded header blocks from the header list and allocates larger buffers when required. Tests cover a response with approximately 70 KiB of headers. ChangesHTTP/3 large-header support
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The large HTTP/3 response-header fix does not require increasing the native client’s HTTP/2 header-list setting. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Noted, the review raised no actionable items. CI is in progress on Buildkite build 116456. The failed claude-find-issues action exited before it ran (is_error on init) and does not relate to this diff. |
|
Updated 4:46 AM PT - Sep 16th, 2026
✅ @robobun, your commit 438ac1ab946ce66af49a02401c3aa5fa934afebc passed in 🧪 To try this PR locally: bunx bun-pr 42905That installs a local version of the PR into your bun-42905 --bun |
|
The updated review still has no actionable items. I merged main into the branch (16a15ea) so the binary-size check compares against the same toolchain: main moved to LLVM 23 after the branch point, and the earlier run reported size deltas in both directions across targets, up to +1.78 MB on freebsd-aarch64, for a diff that adds about 50 lines of C. CI runs again on Buildkite build 116465. |
|
The diff is complete and ready for review. Local verification ran on the branch before the merge from main (LLVM 21 toolchain): the new test fails on the released bun with a timeout and passes on the debug build, and the full serve-http3, node/quic, and fetch-http3 client files pass. After the merge of main, the branch needs clang 23, which this environment does not have, so the local check cannot build either side. CI on Buildkite build 116465 is the verification for the merged state. |
![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 the lsquic patch's headers_max_encoded_size bound (a QPACK literal line is one prefix integer plus one length integer per string, and Huffman is only chosen when shorter, so 2 * LSQPACK_UINT64_ENC_SZ + name_len + val_len per field does not undercount), that every exit from send_headers_ietf after the heap allocation reaches clean: (the early return -1 sits before malloc), and the decode-side relayout in us_quic_hsi_prepare/us_quic_hsi_process (offsets stay relative to h->buf + h->len across a resize, and h->len += is equivalent to the old absolute assignment) — none of those turned up a defect.
Extended reasoning...
Findings are already posted inline (swallowed -1 from the new >65535 guard in Http3Response.h, the pre-existing unbounded total header size on the decode path, and test-coverage nits). This note only records what else was examined and ruled out: the lsquic patch's size bound and heap-buffer ownership on all exit paths, and the per-field base-pointer arithmetic on both the send and decode sides. It is informational, not a correctness guarantee; the vendored-lsquic patch and the QUIC header relayout still warrant a human look alongside the inline comments.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/bun-usockets/src/quic.c— pre-existing, security: a hostile HTTP/3 client can make a Bun.serve server allocate about 2 GB and then spin forever in its event loop with one request whose decoded header set passes 2 GB. us_quic_hsi_prepare at quic.c:522 caps a single field at 64 KB but nothing caps the total, because lsquic only advertises es_max_header_list_size (quic.c:829) and does not enforce it on receipt. Once need exceeds 2^31,ncap *= 2at quic.c:526 wraps to 0 and the while loop never exits. Fix: bound the total decoded size in us_quic_hsi_prepare (fail the block once h->len + space exceeds the context's es_max_header_list_size) for both the server and client contexts. [also at: packages/bun-usockets/src/quic.c:568 - pre-existing, security-relevant: a Bun.serve HTTP/3 peer can make the server buffer a decoded request header set of any size, and after this change that buffer grows without wrapping.]Extended reasoning...
Condition: lsquic's IETF path treats es_max_header_list_size as advertise-only. In this checkout node-quic-accessors.patch:1072 passes it only to lsquic_hcso_write_settings, and patch:1492 logs the peer's value with "we ignore it". The PR description states a 100 x 700 B request header set reaches the handler although the server advertises 16 KB at quic.c:829, which shows the limit is not enforced on receipt. The server does not change es_qpack_dec_max_size, so the decoder's dynamic table stays at lsquic's 4096-byte default. The attacker inserts one ~4 KB entry and then sends indexed field lines of one byte each. For each one lsqpack calls us_quic_hsi_prepare with space of about 4 KB, which passes the per-field check at quic.c:522. h->len grows by about 4 KB per byte received (quic.c:568). lsquic re-extends the stream flow-control window as hq_read consumes the HEADERS frame, so the frame can be arbitrarily long. After roughly 512 KB of compressed input h->len approaches 2^31. At quic.c:523…
Verification: normal (not pre-existing as the candidate labels it: this PR is what makes the growth path reachable). Trigger: a single HTTP/3 client sends one request whose QPACK-decoded header block exceeds 2 GB (feasible: with the default 4 KB decoder dynamic table one 1-byte indexed field line expands to ~4 KB, so ~540 KB of HEADERS payload suffices; static-table-only still gives ~50x). Mechanism, verified…
|
Pushed 92e86de for the review findings: the Bun.serve test compares the received header values byte for byte, and a node:quic case in quic-stream.test.ts covers the send path in node_quic_shim.c (times out on the released bun, passes with the fix). The swallowed -1 in Http3Response.h and the missing total cap on decoded request headers are pre-existing. #42895 handles the first and #35763 the second, so this PR leaves both to them. The rate-limit notice needs no on-demand review. CI runs on the new commit. |
|
The refreshed summary has no actionable items. Nothing changed on my side since 92e86de. CI for that commit is Buildkite build 116480. |
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
This review covers commit 92e86de, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
A note for the merge, from the #42895 side. I merged this branch with #42895 and #42901 locally and ran the tests of all three on a debug build. git reports one conflict between this PR and #42895: both add a test at the same place in The first form of the #42895 tests expected |
This branch has not been deployed
1 participant
Fixes #42887.
Problem
Bun.serve({ http3: true })cannot send a response whose QPACK-encoded header block is larger than 64 KB. The sameResponsegoes out over HTTP/1.1 and HTTP/2. On the canary the client times out. Debug builds abort first:lsxpack_header_set_offset2: Assertion 'val_offset <= LSXPACK_MAX_STRLEN' failed.send_headers_ietf(vendor/lsquic/src/liblsquic/lsquic_stream.c:4120) encodes into a stack buffer ofMAX_HEADERS_SIZE(64 KB) and returns -1 withQWH_ENOBUFfor a larger block. Nothing is written. Upstream master has the same buffer.packages/bun-usockets/src/quic.c. It addressed every field from the start of one shared buffer, and lsxpack offsets are 16-bit.Fix
patches/lsquic/large-header-block.patch:send_headers_ietfcomputes an upper bound of the encoded block from the header list (name, value, and two QPACK integers per field). A block under 64 KB keeps the stack buffer. A larger one uses a heap buffer that is freed atclean:.quic.c: eachlsxpack_headerpoints at its own slice of the shared buffer with offsets from 0, on the send path (us_quic_stream_send_headers) and the decode path (us_quic_hsi_prepare,us_quic_hsi_process). The send-side hunks inquic.candnode_quic_shim.care the same as in Bun.serve(http3): reset the stream when lsquic refuses the response headers or a body write #42895, so either PR merges cleanly after the other. This relayout becomes unnecessary once node:quic(h3): build lshpack/lsqpack with 32-bit header lengths so large request headers don't abort the connection #35741 builds lsxpack with 32-bit lengths.size_tandunsigned, so a block over 64 KB goes through unchanged.test/js/bun/http/serve-http3.test.tsandtest/js/node/quic/quic-stream.test.ts(one new case each, 100 headers of 700 bytes, both time out on main). Also the fullserve-http3file,test/js/node/quic/, and the threefetch-http3-*client files.Background
send_headers_ietfbefore it writes to the stream.lsxpack_headeris the struct lshpack, lsqpack, and lsquic share to describe one field: abufpointer plus 16-bit name and value offsets and lengths. An offset that does not fit trips an assertion in debug builds and wraps in release builds.MAX_HEADERS_SIZE.Notes
maxHeaderLength: 1 << 20.us_quic_stream_send_headersreturns -1 for it, andHttp3Response.hdoes not act on that value yet, so such a response does not go out. Bun.serve(http3): reset the stream when lsquic refuses the response headers or a body write #42895 makes the -1 fatal with RESET_STREAM. Widening the lengths is the scope of node:quic(h3): build lshpack/lsqpack with 32-bit header lengths so large request headers don't abort the connection #35741 and Bun.serve(http3): lift the 64 KB QPACK prepare_decode cap so a large request header does not abort the connection #35763.SETTINGS_MAX_FIELD_SECTION_SIZEon the send side (lsquic_full_conn_ietf.c, "we ignore it"). This PR does not add enforcement of it.node_quic_shim.c, state the motivation and the related PRs in the body, execution review found no defect).