Repository navigation
Conversation
|
Reproduced on bun 1.4.3-canary.1 in three ways. Each one leaves fields in the HPACK encoder table that the peer never receives.
With this branch (88481d8) all three decode right. The 54 new rows in Status: draft. The branch is rebased on main and the code is complete. A self-review of the final diff is in progress. Not fixed here: a field over 65536 bytes still ends the session (see Downsides and Notes in the PR body). |
|
Updated 7:29 PM PT - Oct 7th, 2026
✅ @robobun, your commit 88481d8b8a19b15de8671bcbf7e428b6c635125b passed in 🧪 To try this PR locally: bunx bun-pr 41520That installs a local version of the PR into your bun-41520 --bun |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughHTTP/2 header serialization now validates fields before HPACK mutation across request, trailer, and PUSH_PROMISE paths. Request size checks and native request error handling were updated. Regression tests cover invalid headers and session continuity. ChangesHTTP/2 header encoding
Suggested reviewers: Merge Risk: ⚪ Minimal · up to HTTP/2 header handling now validates complete header lists before HPACK encoding, preventing invalid fields from corrupting later connection activity. No concrete current-head merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/http2.ts— nit: removing theprocess.nextTick(emitErrorNT, ...)call inClientHttp2Session#request()'s catch block leavesemitErrorNT(line 447) with no callers in this file — it is now dead code. Fix: delete theemitErrorNTfunction in the same PR (REVIEW.md: "Delete dead code in the same PR that makes it dead… helpers whose last caller you rewired").Extended reasoning...
Grep of
emitErrorNTin src/js/node/http2.ts after the diff returns only line 447 (the definition); the sole call site at the old line 6269 was removed by this change. The identically-named helper insrc/js/internal/streams/destroy.tsis a separate module-local function with a different signature and does not reference this one. Nothing else in the module (or the codebase) imports or calls http2.ts's three-argumentemitErrorNT, so the function is unreachable after merge. REVIEW.md lists dead-code deletion as required scope for the PR that orphans the helper.Verification: nit — The diff removes the sole call site:
- process.nextTick(emitErrorNT, this, e, this.#connections === 0 && this.#closed);at src/js/node/http2.ts:6269 (base). After the change, grep ofemitErrorNTin src/js/node/http2.ts returns only line 447, the definitionfunction emitErrorNT(self: any, error: any, destroy: boolean) { ... }. This is a module-local (non-exported) function with…
|
Both review findings are addressed in 0bffe36: Also ran node's |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
#41614 is stacked on this branch. It adds node's value semantics (undefined skipped, null stringified, symbol keys never on the wire) on top of the HeaderList change here, and removes the "null value" row from the poison matrix because null is no longer a throw. |
f7dc184 to
90f334a
Compare
|
A note for whoever merges this: there is an overlap with #43474 (stacked on #43440). #43474 now stages every header block in the native #43474 does not touch |
…ees it sendTrailers, additionalHeaders, respond, pushStream and request validated each field and encoded it into the shared HPACK dynamic table one at a time. A later field that failed validation threw after the earlier fields were already in the table and never sent, so the peer's table fell behind and every later header block on the connection decoded against the wrong entries. The JS walk now collects the validated fields into a HeaderList and the encode runs once the whole object is known. request() also checks maxSendHeaderBlockLength against the pre-compression bound, as nghttp2 does, and runs the option and memory-limit checks before the encode, so a refused block never touches the table either. A client request() that throws no longer emits a session 'error' as well.
|
This PR is not complete. I found a case it does not fix. What still breaks. A header call whose HPACK encode fails on a later field leaves the earlier fields of that call in the encoder table. The encoder refuses a field when the name plus the value is longer than 65536 bytes ( Measured. Server: main bbdc5a5 plus this PR, debug build. Client: node v26.3.0. One connection, three streams in flight, answered in the same tick as the failed call. 3 of 3 answers arrive with wrong headers and no error, and they carry a What the PR body said. The Notes said an encoder failure ends the session, so this case needed no check before the encode. I did not test that statement. It is wrong. The body is corrected. What does hold. The validation cases (invalid name, CR/LF, The branch is rebased on main locally. There was one conflict hunk in each of |
The cork buffer is shared by every session on the thread. cork() flushes the session that owns it through that session's transport. When the transport is a JS Duplex, its _write can make a header call on the session that is taking the buffer over. cork() then replaced the owner and reset the offset, so the frames of that call were dropped while their fields stayed in the HPACK table. A block that was encoded before the hand-over also went out behind a block that was encoded after it. cork() now reads the slot again after each forced uncork and keeps what a re-entrant call corked. The header encode takes the cork before it encodes, so no JS runs between the encode and the last byte of the block. A native socket on top of a JS Duplex goes through the unit-assembling write, like a session with no native socket, so a header block larger than the cork is handed over whole.
The HPACK encoder refuses a field whose name plus value is over 65536 bytes. The walks encoded field by field, so a block with such a field left its earlier fields in the dynamic table. The call does not throw: the session error arrives from the event loop, and header blocks that other streams wrote in the same tick went out with shifted indices. lshpack_wrapper_encode_block checks every field of a block before the first one reaches the encoder, then encodes the staged bytes in place. HPACK::encode_block is the only encode the runtime crate can call: HPACK::encode is crate-private to bun_http now. The walks stage into a per-VM scratch (RareData), so a header call makes no allocation once the scratch is warm. The h2 engine stages and encodes the same way. request() checks maxSendHeaderBlockLength on the pre-compression bound. A refused server response now resets its stream with FRAME_SIZE_ERROR, as node does, and a refused 1xx block leaves the stream open. Before, the peer got no frame for it.
90f334a to
57fa470
Compare
…s header block senders Shorten the comments the header block encode added.
…test node closes the session after a frame error, so the 1xx case gets a session of its own, and the test no longer asks for a later request on the same session.
clippy::chunks_exact_to_as_chunks rejects chunks_exact with a constant size.
robobun
changed the title
This branch has not been deployed
2 participants
Problem
node:http2header call that fails part-way leaves its earlier fields in the HPACK encoder table, unsent. Later blocks decode against shifted entries: another stream'sset-cookie, orERR_HTTP2_ERROR Protocol error.send_trailers,push_promiseandrequest(src/runtime/api/bun/h2_frame_parser.rs) encoded field by field. A later field could fail validation or exceed the encoder's 65536 bytes.Fix
lshpack_wrapper_encode_blockchecks every field, then encodes the block.bun_runtimehas no per-field encode.cork()keeps frames that transport JS queues during a hand-over. The encode corks first.test/js/node/http2/node-http2.test.js(54 new rows, 51 fail on main),test/js/node/http2/, node's 256test-http2-*.js.Background
Downsides
maxSendHeaderBlockLengthcounts the uncompressed bound, like node. A block that fits only after compression is refused. A refused response gets aFRAME_SIZE_ERRORreset.mi_mallocless, +1.3% instructions (28,324 against 27,947, 7-fieldrespond()). Per VM: +72 bytes, up to 128 KiB kept..text: +2,304 bytes.Notes
Three ways a block failed after its first fields were in the table
undefined,null, aSymbol, atoStringthat throws.maxSendHeaderBlockLengthcheck inrequest()also ran after the encode.cork()flushes the previous owner of the cork buffer through its transport. When that transport is a JSDuplex, its_writecan make a header call on this session.cork()then reset the buffer offset and dropped those frames, after their fields were in the table.Repro for the first way (bun 1.4.3, bun client and bun server):
Before:
After:
What a refused block does now
respond(),additionalHeaders(),pushStream(),request(), compatwriteHead()andwriteEarlyHints()with a field over 65536 bytes: the session still ends withERR_HTTP2_SESSION_ERROR: Session closed with error code 9one tick later, as before. Blocks that other streams send in that tick now decode right.sendTrailers()with such a field:frameError, the stream ends withFRAME_SIZE_ERROR, then a graceful GOAWAY, as before.maxSendHeaderBlockLength: the check uses nghttp2's bound (12, plus 12 per field, plus the name and value bytes, plus 5) and runs before the encode. A refused server response dispatchesframeErrorand resets the stream. A refused 1xx block leaves the stream open for the final response. The client side is as before, with the new bound.ClientHttp2Session.request()is no longer reported a second time as a session'error'. Nothing reached the wire, and node reports the throw only.Measurements
Release builds of main (bbdc5a5) and of this PR, linux x64, same toolchain. The container has no
perf,valgrind,strace,ltraceorbloaty. The counts come from gdb: breakpoints on the allocator entry points and single steps, counted only inside the host function, in steady state.mi_malloccalls, bytes requested):respond(), 7 fields: 19 (17,136 B) on main, 18 (752 B) on the PRrequest(): 27 (17,328 B), then 26 (944 B)sendTrailers(), 2 fields: 7 (16,688 B), then 6 (304 B)pushStream(), 5 fields: 15 and 1mi_realloc, then 14 and 0additionalHeaders(): 9, then 8respond(), 7 fields: 27,947, then 28,324 (+1.3%)request(): 33,361, then 33,735 (+1.1%)sendTrailers(): 14,183, then 14,132 (-0.4%)pushStream(): 21,602, then 21,633 (+0.1%).text: 65,382,485 B, then 65,384,789 B (+2,304 B). The stripped binary is 88,864,328 B on both.lshpack_wrapper_*symbols: 2, then 3.size_of::<H2HeaderScratch>()is 72, soRareDatagrows by 72 B.H2FrameParserstays at 1,544 B. The slot keeps a scratch only when each of its two buffers is at most 64 KiB.write()of a 9-byte chunk when the session owns the cork: 79 instructions, then 71.Tests
test/js/node/http2/node-http2.test.jshas 54 new rows. 51 fail on bun 1.4.3-canary.1.additionalHeaders,respond, compatres.end, server and clientsendTrailers,pushStreamand clientrequest. Each row makes a clean request, the failing call, and a clean request on the same session. The invalid-name rows ofrespond()and of clientrequest()pass on main.request(). One row reads raw frames: a field of exactly 65536 bytes is sent, and the next block is indexed against it. That row passes on main.Also run: all of
test/js/node/http2/and node's 256test-http2-*.jsfiles on a debug build and on a release build of the PR, and the source lints intest/internal/. On the debug build in my container, 15 older tests of this file that start a child process need more than the 5 s default timeout, on main too. They pass with--timeout 120000.Not in this PR
encode_blockto take every field (a literal without indexing for a field lshpack refuses).sendTrailers()block still ends the other streams of the session early, withEND_STREAM. The table stays in sync. Main has the same bug.fetch(src/http/h2_client/) keeps its per-field encode. After a failed encode it opens no new stream on that connection (encoder_poisoned), so no later block uses the shifted table.src/runtime/api/bun/h2/(connection.rs,hpack.rs) uses the block encode too. No JS path calls that engine yet. Its unit tests are compile-checked only (cargo check -p bun_runtime --lib --tests).Related PRs
respond()exceedsmaxSendHeaderBlockLength. This PR moves that check before the encode, so it needs the same reset on the server side and has it. The PR that merges second needs a rebase. The same holds for node:http2: refuse a header block over the default send limit, like node #43474, node:http2: fail only the push when a pushStream() header block is over the send limit #43619 and node:http2: refuse a trailer block over the send limit #43632, which are stacked on node:http2: reset the stream when respond() exceeds maxSendHeaderBlockLength #43440 and touch the same functions.History
The first version of this PR (September, 90f334a) fixed the validation case only. Its text said that an encoder failure needs no pre-check, because it ends the session. That statement was not tested and it is wrong: see the second way above. This version replaces it.
[human-review] gate passed · iteration 0 · 3 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 header-serialization paths for
request,sendTrailers,additionalHeaders, andpushStreamvalidated and HPACK-encoded each field one at a time, so when a later field failed validation the call threw after earlier fields had already been inserted into the connection's shared dynamic table without ever being sent, leaving the peer's decoder out of sync and causing later header blocks to decode onto the wrong stream or trigger a COMPRESSION_ERROR GOAWAY. The fix collects and validates the complete header list into a private buffer before the encoder is touched, then performs a single…