Repository navigation
node:http2: fail only the push when a pushStream() header block is over the send limit - #43619
Conversation
…er the send limit push_promise() encoded each field as it read it and had no send limit check. A field over 64 KiB failed in the HPACK encoder and ended the whole session with COMPRESSION_ERROR. The fields encoded before the failure stayed in the encoder table, so the parent response could carry an index the peer never received. push_promise() now stages the block in HeaderList and checks it with over_send_limit(), the rule request() already applies, before anything reaches the encoder. For a refused block it closes the reserved stream without an RST_STREAM and reports it to pushStream(). pushStream() then does what node does: the callback still gets the reserved stream, the parent gets 'frameError' (PUSH_PROMISE, FRAME_SIZE_ERROR) and the graceful close, and the reserved stream fails with INTERNAL_ERROR from setImmediate. A stream the peer never saw announced sends no frames.
|
Status: the fix is pushed and waits for a maintainer review. This PR is stacked on #43474 (which is on #43440). CI on 2e332fe (build 119447). The build is finished: 180 jobs passed, with the test lanes of every platform among them. The one red job is How to reproduce. Start an stream.pushStream({ ":path": "/pushed", "x-big": Buffer.alloc(90000, "A").toString() }, (err, push) => push.on("error", () => {}));
stream.respond({ ":status": 200 });
stream.end("x");Send one GET with a raw frame client (or
The tests are in |
|
Updated 5:13 PM PT - Sep 21st, 2026
❌ @robobun, your commit 2e332fe has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 43619That installs a local version of the PR into your bun-43619 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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 findings (all in the http2.ts layer), I also checked the Rust side of src/runtime/api/bun/h2_frame_parser.rs: the over_send_limit extraction preserves the request() semantics exactly (same two match arms, StreamPriority::BYTE_SIZE still counted for HEADERS, 0 for PUSH_PROMISE), and the refusal path returns before any field reaches encode_header_into_list, so the HPACK dynamic table stays in sync with the peer; the enter_stream_dispatch guard is scoped to the if let block and drops before the early return.
Extended reasoning...
Inline findings were reported, so this is the brief ruled-out note only. The Rust diff was read in full: the staged HeaderList replaces the incremental encode loop, the new helper is a verbatim lift of the prior request() check, and the -1 compression-error path is unchanged in behaviour (now iterating staged.iter() instead of encoding inline). The confirmed findings concern the TS-side lifecycle of the kNeverAnnounced stream (respondWithFD sink bypassing nativeForStreamFrames, missing _final callback, rstCode overwrite on close, unconditional onServerFrameError, and the missing end() for endStream/HEAD pushes) and warrant human attention before merge.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/http2.ts— A server that answers a refused push with respondWithFD() can crash with an uncaught 'Invalid stream id' Error thrown from the file sink. The sink at src/js/node/http2.ts:3086 reads session[bunHTTP2Native] directly, not nativeForStreamFrames, and its only guard is stream.destroyed || stream.closed at http2.ts:3087. Between the callback's nextTick and failUnannouncedPush's setImmediate the native entry for the kNeverAnnounced id can be evicted by a socket read, and native.writeStream at http2.ts:3091 then throws. Fix: route the sink's write (and final) through nativeForStreamFrames so a never-announced stream drops the chunk, matching _write/_writev. [also at: src/js/node/http2.ts:2468 - User code that reads pushedStream.state (or endAfterHeaders) after an await inside the pushStream callback gets a bare Error 'Invalid stream id' instead of node's state object.]Extended reasoning...
The finder settled this by argument (basis argued, 5 finders). push_promise refuses the block and calls free_resources:: on the promised stream (h2_frame_parser.rs:6284), which queues the id in pending_engine_stream_closes. The next rewrite_read at dispatch depth 0 removes the map entry (h2_frame_parser.rs:3695-3708). pushStream delivers the stream on nextTick (http2.ts:3307) and destroys it only on setImmediate (http2.ts:3305). In the callback the user calls push.respondWithFD(fd, headers). respond() is silently skipped (headersSent = true), closeWritableForFileResponse runs, then fs.fstat completes on the threadpool and doSendFileFD creates the read stream and pipes it into the sink (http2.ts:3112). If a client frame (WINDOW_UPDATE, PING, a pipelined request) arrives in that window the entry is evicted; a fast page-cache read then reaches sink.write before the immediate: stream.destroyed is false, native.writeStream(stream.id) hits the missing-entry branch (h2_frame_parser.rs:5970-5971) and throws 'Invalid…
Verification: normal (narrow timing race, but the outcome is an uncaught exception that kills the server process). Trigger: a refused pushStream() whose callback answers with respondWithFD()/respondWithFile(), when a depth-0 socket read on that session lands before the failUnannouncedPush immediate runs (e.g. pushStream called from another socket's callback or from a setImmediate, so the client socket is…
A HEAD or endStream push over the send limit now reaches the callback with its writable ended, as a sent push does, so its failure is not an abort. A refused push that the callback closes keeps the code of that close() and ends without an 'error'. Both match node v26.3.0. The write guards for a never-announced stream move from Http2Stream into the ServerHttp2Stream overrides. A refused client request is destroyed on the next tick and needs none, and it emits 'finish' again as on the base. The file response sink and the state getters also skip the native entry of a never-announced stream, which can be gone before the stream fails.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reply to the review finding outside the diff (the The file sink and the getters: fixed in 6f882df. The race is narrow. It needs two thread pool completions and a read on the same session before the
I have no deterministic test for the race. Inline threads. All measured against node v26.3.0 with a raw frame client.
The multi-line comments that the comment check flagged are one line each now (c550f99). |
There was a problem hiding this comment.
I re-reviewed the two follow-up commits and found no new bugs; because this changes the PUSH_PROMISE path in both the Rust frame parser and the node:http2 stream state machine, a human look is still worthwhile.
What was reviewed:
- The follow-ups fold the refused push into the shared Delivered/HEAD/endStream path and keep a user-closed stream's rstCode in
failUnannouncedPush; tracedclose()/destroy()throughmarkStreamClosed->markWritableDone, which is what drains the callback the newServerHttp2Stream._finalstashes for a never-announced stream. - Every remaining native entry point on a
kNeverAnnouncedserver stream (respond,additionalHeaders,_write/_writev, thedoSendFileFDsink,state/endAfterHeaders,_destroy'srstStream,sendTrailersvia the WantTrailer gate) is guarded, so nothing is written for an id the peer never saw;setStreamContext(id, undefined)after nativefree_resourcesonly drops the sctx root, so the double release is benign. over_send_limit()is a faithful extraction of therequest()rule (priority bytes for HEADERS, none for PUSH_PROMISE), and the staged block never reaches the HPACK encoder on refusal, which the client-decodes-after-refusal test pins.- The one design point from the earlier round the author chose to keep as-is is closing the parent and session regardless of whether a
frameErrorlistener exists; that divergence from node's listener-count gate is pre-existing at the HEADERS site and is a judgment call for a maintainer.
Extended reasoning...
Overview
The PR touches three files: src/runtime/api/bun/h2_frame_parser.rs (a new over_send_limit() helper shared by request() and push_promise(), staging of the PUSH_PROMISE block in HeaderList before encoding, and a -2 sentinel that closes the promised native stream without RST_STREAM), src/js/node/http2.ts (the kPushPromiseOverSendLimit branch in pushStream(), failUnannouncedPush scheduled on setImmediate, onServerFrameError shared with the native frameError handler, and kNeverAnnounced guards on every path that would otherwise write a frame for the reserved id), and ~300 lines of new tests in test/js/node/http2/h2-conformance.test.ts using the raw-frame client. Since my previous review, two follow-up commits removed the nativeForStreamFrames helper in favor of explicit guards, moved the refused branch so it shares the Delivered/HEAD/endStream handling with a sent push, made failUnannouncedPush respect a stream user code already closed, and added tests for the HEAD/endStream and close()/close(CANCEL) cases.
Security risks
No auth, crypto, or permission surface is involved. The input to the new native code is header data supplied by the server application, not by the peer; the staging path uses try_reserve and maps allocation failure to a thrown JS error rather than unwinding. The refusal path frees the promised stream's resources via the same free_resources::<false> used by the client-side refusal in request(), and the JS side's kReleaseUnannouncedStream only removes the sctx root, so there is no double free. The change reduces a peer-observable failure mode (a session-wide GOAWAY plus desynchronized HPACK tables) rather than adding one.
Level of scrutiny
High. This is protocol state-machine code split across Rust and TypeScript where a missed guard means writing a frame on a stream id the peer considers idle, which is a connection error. I traced every native call site reachable from a ServerHttp2Stream (respond, additionalHeaders, _write, _writev, _final, the doSendFileFD sink, sendTrailers, _destroy's deferred rstStream, the state and endAfterHeaders getters) and each is gated on kNeverAnnounced or on a state flag that a never-announced stream cannot reach. The _final override stashes the callback in bunHTTP2StreamFinal, which markStreamClosed drains from both close() and _destroy, so end() does settle once the stream is closed or destroyed. Because the timing semantics (nextTick callback, nextTick frameError, setImmediate failure) are asserted against node measurements the PR author reports rather than something I can reproduce here, a maintainer familiar with the http2 compat layer should still confirm the ordering is the one they want to pin.
Other factors
The bug hunt ran to a dry streak with no findings. Two of my four earlier inline points were addressed by the follow-ups (closed-stream rstCode; HEAD/endStream on the refused path), the _final point was reworked into the stash-and-drain shape, and the frameError listener-count gating point was resolved by the author without a code change; that last one is a compatibility judgment rather than a defect, and it applies equally to the pre-existing HEADERS refusal site, so I did not re-raise it inline. The new tests cover exact limit boundaries on both sides (65536/65537-byte field, at/one-over a user limit), both HEAD and endStream, close()/close(CANCEL), an unanswered parent with a stream in flight, and a real node:http2 client decoding responses after the refusal, which is the observable consequence of keeping the refused block out of the encoder. The tests use try/finally rather than using for server cleanup, which is consistent with the rest of that file.
|
One correction to the summary above, for the maintainer who reads it: the close with no Node v26.3.0 has no listener-count gate. The review threads are resolved. The PR waits for CI and a maintainer review. |
|
A note for when this stack merges main. main (4af1842) now typechecks The line is |
|
@robobun Node v26.3.0 sends the graceful GOAWAY(NO_ERROR, last=3) twice here: once from |
Node sends the graceful GOAWAY twice: from session.close() after the frame error, and again from Http2Session::Close. Bun sends it once. The tests now drop one repeat of the first GOAWAY, so they pass with one or two and still fail with a third. The control for a field of exactly 65536 bytes says in its title that node refuses that block.
|
@cirospaciari Done in 2e332fe. The tests now accept one or two graceful GOAWAY frames. What changed. The block has a Checked on both runtimes. I ran the same two scenarios, with the same helper and the same expected values, as a plain script on node v26.3.0 and on the debug build: The second scenario is the unanswered parent with a stream in flight. Node gives Two controls do not pass on node, on purpose. Their titles say so:
They pin the limit rule from #43474: with |
There was a problem hiding this comment.
Re-reviewed the latest push; no new bugs found, and the follow-up commits address what I raised earlier. Since this changes native stream teardown in the frame parser plus the server-side push state machine, a maintainer look is still worthwhile.
What was reviewed this round:
- The refused-push path now falls through to the HEAD/endStream
end()andDeliveredmarking, andfailUnannouncedPushskips the rstCode overwrite for a stream user code already closed. - The
_finalcallback a never-announced push parks inbunHTTP2StreamFinalis drained bymarkStreamClosed -> markWritableDonefrom bothclose()and_destroy, sofinish/end(cb)settle. - Native:
push_promise()closes andfree_resources::<false>'s the promised stream the same wayrequest()'s client refusal does; every JS path that could reach native for that id (respond,additionalHeaders,writeStream,rstStream,getStreamState,getEndAfterHeaders, the sendfile sink) is gated onkNeverAnnounced, and the redundantsctxremove inkReleaseUnannouncedStreamis a no-op after the native free.
Extended reasoning...
The diff touches src/runtime/api/bun/h2_frame_parser.rs (a shared over_send_limit helper, push_promise staging headers into HeaderList before encoding, and a -2 sentinel returned after closing the reserved stream without RST_STREAM), src/js/node/http2.ts (pushStream handling of the sentinel, kNeverAnnounced gates on several ServerHttp2Stream paths, a shared onServerFrameError helper, and failUnannouncedPush), and 16 new tests in test/js/node/http2/h2-conformance.test.ts. No auth, crypto, or injection surface is involved; the security-relevant part is peer/user-sized header handling, which is bounds-checked before the HPACK encoder runs. The change is moderately complex, involves raw-pointer stream teardown in native code and cross-layer ordering (nextTick/setImmediate) that mirrors nghttp2, and a maintainer (cirospaciari) commented on the previous revision, so approval is left to a human even though this run found no defects and the prior inline findings appear addressed in commits 6f882df and c550f99.
|
A heads-up for the next merge of the base branch (#43474, now at e7d15ae). A review asked for Node's default there, so two places that this PR also touches changed:
The closed-stream check that 1efae9f added to the native |
This branch has not been deployed
2 participants
Problem
node:http2server callsstream.pushStream()with a header field over 64 KiB. Bun ends the whole session:GOAWAYcode 9,ERR_HTTP2_SESSION_ERROR: Session closed with error code 9on every stream. Node fails only the push.push_promise()(src/runtime/api/bun/h2_frame_parser.rs) encodes each field as it reads it, with no send limit check. The HPACK encoder refuses the field, and the session ends.ERR_HTTP2_ERROR: Protocol erroron bun's own client).Fix
push_promise()stages the block inHeaderListand checks it withover_send_limit(), the rulerequest()has in node:http2: refuse a header block over the default send limit, like node #43474. A refused block never reaches the encoder. The reserved stream closes with noRST_STREAM.pushStream()then does what node does. The callback gets the reserved stream. The parent getsframeError(5, 6) and the graceful close from node:http2: reset the stream when respond() exceeds maxSendHeaderBlockLength #43440. The reserved stream fails withNGHTTP2_INTERNAL_ERROR.kNeverAnnouncedsends no frames, because the peer never saw its id.test/js/node/http2/h2-conformance.test.ts(16 new tests, 12 fail on the base, 4 are controls). Alsotest/js/node/http2/and node'stest-http2-*.js.Background
HeaderList, the limit rule and theframeErrorclose.pushStream()reserves an even stream id. The nativepushPromise()announces it with a PUSH_PROMISE frame.maxSendHeaderBlockLengthis set. When it is unset, only a field over 65536 bytes, which the encoder cannot emit.Notes
Where this comes from. No user reported it. It came out of differential tests of
node:http2against node v26.3.0, after #43474 listedpushStream()as not covered.Measured. Raw frame client, plain TCP, default options. The handler calls
pushStream()with a 90000-byte field, thenrespond()andend("x")in the same tick.1 HEADERS,1 DATA END_STREAM,GOAWAY 0 last=1(sent twice)(null, push 2), parentframeError 5 6 1, pushERR_HTTP2_STREAM_ERRORNGHTTP2_INTERNAL_ERRORwithrstCode 2, parentrstCode 0, session close1 HEADERSwhose block starts88 be(index 62, which the peer never received),1 DATA END_STREAM,2 HEADERS,2 DATA END_STREAMfor a stream that was never promised,GOAWAY 9ERR_HTTP2_SESSION_ERRORcode 9 on the session, the push and the parent1 HEADERS(the same bytes as node),1 DATA END_STREAM,GOAWAY 0 last=1frameErrorhas two arguments, as everywhere in bunWhat is refused now that main sends. With the option unset: nothing. The rule refuses only a field over 65536 bytes, and that push ends the session on main. A block of many small fields over 64 KiB in total still goes out, although node refuses it (a test pins this, the same choice as #43474 for
respond()). With the option set: main ignores it for pushes. A push over the user's limit is now refused, as in node. The bound for PUSH_PROMISE has no priority bytes:12 + 12 * fields + names + values. Checked against node with an explicit and a default limit of 65536: a 65407-byte value is sent, 65408 is refused. Both sizes are tests, and so are a low limit that refuses a block and a higher limit that sends the same block.Timing. The callback runs on
process.nextTick, as before.frameErrorfollows on the next tick, so it always comes before the parent'sclose. The reserved stream fails fromsetImmediate. nghttp2 closes it when the session next sends, which is after the ticks and microtasks of the turn. A callback that answers the push after anawaitof a settled promise must not throw. Measured on node:respond()from a microtask or anextTickin the callback returns,respond()from asetImmediatethrowsERR_HTTP2_INVALID_STREAM. Bun now does the same, and a test covers the microtask case. In the same-tick case the parent'sclosecomes before the push'serrorin bun, and after it in node, because bun writes the parent's response at once.Why the JS guard and not only the native state. The native side closes and frees the reserved stream. Its map entry goes away on a later read, and
request()would create a new stream for the id. The reserved stream lives untilsetImmediate, so theServerHttp2Streampaths skip the native side for akNeverAnnouncedstream:respond(),additionalHeaders(),_write,_writev,_final, the file response sink, and thestateandendAfterHeadersgetters. A dropped write calls its callback at once._finalkeeps its callback untilclose()or the failure settles it, so there is nofinish(node has none either) and anend()callback getsERR_HTTP2_STREAM_ERROR, as in node.Http2Streamitself is unchanged: a refused client request is destroyed on the next tick and still emitsfinish, as in node. The raised limit case (-1) sets the same flag, so it no longer writes HEADERS and DATA for a stream that was never promised before itsGOAWAY 9.A refused push stays like a sent one until it fails. Measured on node: a HEAD or
endStreampush over the limit reaches the callback withwritableEnded === true(andheadRequest === true), and its failure emits noaborted. A callback that callsclose()orclose(NGHTTP2_CANCEL)getsaborted,finishandclosewith that code, and noerror. Bun does the same, and four tests cover it. Node reports the frame error with or without aframeErrorlistener:OnFrameNotSentin v26.3.0 has no listener check, and the wire is the same with no listener anywhere.The graceful GOAWAY count. Node sends
GOAWAY 0twice: fromsession.close()after the frame error, and again fromHttp2Session::Close. Bun sends it once. The tests drop one repeat of the first GOAWAY, so they pass with one or two and still fail with a third. The same scenarios, with the same expected values, pass as a plain script on node v26.3.0 (two GOAWAY frames on the wire) and on bun (one). Two controls do not pass on node on purpose, and their titles say so: they pin the rule that an unset limit refuses only a field over 65536 bytes.Other cases measured against node. A parent that never responds:
RST_STREAM 6on it,GOAWAY 0, and a stream in flight still completes (a test). A parent that responds later:ERR_HTTP2_STREAM_ERRORNGHTTP2_FRAME_SIZE_ERROR,rstCode 6.respond()now andend()later:HEADERS, an emptyDATA END_STREAM,GOAWAY 0, the same wire as node.Not in this PR
maxSendHeaderBlockLength: 100000and a 90000-byte field: node sends that push, bun cannot encode the field, and the session error stays.delivers the reserved push stream and fails the session when its headers cannot be encodedstill passes.sendTrailers()still has its own encode loop.frameErrorargument (the stream id).push_promise()closes it here at the refusal. The two do not conflict in behaviour.push_promise()the same way for the validation order. It needs a rebase over this stack.Self-review. A review of the first version (a standalone branch on main with its own
HeaderListand a default limit of 65536 for pushes only) asked for a restack: oneHeaderList, one limit rule and oneframeErrorclose for the three senders, no new cap, and the base named here. This version does that. It also adds the two tests the review asked for: a higher limit sends the block a lower limit refuses, and a real client decodes a second response on the same connection after the refused push.Suites run with the debug build:
test/js/node/http2/h2-conformance.test.ts(101 pass),test/js/node/http2/(608 pass, 6 skip, and 3 subprocess tests that time out under the load of this container and pass alone), and 261 vendoredtest/js/node/test/{parallel,sequential}/test-http2-*.jsfiles (all exit 0).[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file