node:http2: rewritten inbound engine, batched write path, server push, +290 node v26.3.0 tests (79% passing) - #31584
Conversation
|
Updated 6:04 PM PT - Jun 17th, 2026
❌ @cirospaciari, your commit 557f409 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31584That installs a local version of the PR into your bun-31584 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
92e6f86 to
8182ad9
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/api/bun/h2_frame_parser.rs:8377-8385—read()no longer pins the input ArrayBuffer (the previousas_pinned_arraybuffer()/unpin()was replaced with a bareas_array_buffer()), butrewrite_read()'s empty-tail fast path passes the borrowedbyte_slice()directly intoengine.receive(), which loops over it and synchronously dispatches Sink callbacks (on_ping/on_data/on_headers_complete) into JS user code between frames. If a user handler.transfer()s the input chunk's ArrayBuffer (reachable via acreateConnectionDuplex), the&[u8]the engine continues iterating over now dangles — UAF/garbage parse. The PR also deletes the regression test for exactly this scenario fromnode-http2.test.js. Restore the pin/unpin aroundrewrite_read()(or copy the bytes before dispatch whenrewrite_tailis empty).Extended reasoning...
What the bug is
H2FrameParser::read()(the JS-fed inbound path used whenoptions.createConnectionreturns a userDuplex) takes a JS Buffer/ArrayBuffer, borrows its backing memory as a&[u8], and hands it to the new frame engine. The previous implementation explicitly protected that borrow:let array_buffer = buffer.as_pinned_arraybuffer(global_object); // ... read_bytes() loop ... if let Some(array_buffer) = &array_buffer { array_buffer.unpin(); }
Per
src/jsc/JSValue.rs:954-961,as_pinned_arraybuffer"pins the backingJSC::ArrayBufferfirst so it cannot be detached" — i.e. while pinned,ArrayBuffer.prototype.transfer()/structuredClone(..., {transfer})cannot move the bytes out from under the borrowed slice.The new implementation (lines 8378–8386) drops the pin entirely:
let buffer = args_list.ptr[0]; buffer.ensure_still_alive(); if let Some(array_buffer) = buffer.as_array_buffer(global_object) { this.rewrite_read(array_buffer.byte_slice()); ... }
ensure_still_alive()only protects against GC; it does not prevent detachment.The code path that triggers it
rewrite_read()has two branches. Whenrewrite_tailis empty (the common case — every chunk that starts on a frame boundary), it does no copy:if self.rewrite_tail.get().is_empty() { let consumed = { ... engine.receive(self, bytes).consumed }; ... }
Connection::receive()(src/runtime/api/bun/h2/connection.rs) then loops:loop { let remaining = &bytes[offset..]; ... let payload = &remaining[FRAME_HEADER_SIZE..total]; if self.dispatch(sink, &hdr, payload) { ... } offset += total; }
dispatch()→handle_ping/handle_data/handle_headers→sink.on_ping/sink.on_data/sink.on_headers_complete. TheSinkimpl forH2FrameParser(e.g.on_ping) callsself.dispatch_with_extra(JSH2FrameParser::Gc::onPing, ...), which synchronously invokes the JS handler —session.emit('ping', payload)inhttp2.ts. After that JS callback returns,receive()re-derives&bytes[offset..]and continues parsing the next frame from the same slice.Step-by-step proof
This is exactly the scenario the deleted regression test (
'http2 client keeps parsing a socket chunk whose ArrayBuffer is transferred by a frame event handler'intest/js/node/http2/node-http2.test.js) exercised:http2.connect('http://localhost', { createConnection: () => socket })with a userDuplex. Thesocket.on('data', ...)listener installed byhttp2.tscallsparser.read(chunk)—chunkis the exact Buffer the user pushed.- User pushes
pingChunk = Buffer.from(chunkArrayBuffer)containing two PING frames back-to-back (sorewrite_tailis empty → no-copy path). read()→as_array_buffer()(unpinned) →byte_slice()→rewrite_read(bytes)→engine.receive(self, bytes).- The engine parses frame 1 (PING), calls
sink.on_ping()→dispatch_with_extra(onPing, ...)→ JSclient.on('ping', payload => { ... })runs synchronously. - The user handler calls
chunkArrayBuffer.transfer()andnew Uint8Array(moved).fill(0xff). With the pin removed, transfer succeeds: the backing store is moved to a new ArrayBuffer the user owns and may free/overwrite; the original is detached. - The handler returns.
receive()continues withlet remaining = &bytes[offset..]— butbytesstill points at the old backing store, which is now detached. The engine reads frame 2's header and payload from memory the application owns and has overwritten with0xff(or that the allocator has reclaimed).
The deleted test asserted
PINGS:["4141…","4242…"]— i.e. the second ping's original payload survives the mid-parse transfer — which only held because the pin made step 5 throw (try { ... .transfer() } catch { /* runtime may refuse */ }).Why nothing else prevents it
- The
elsebranch ofrewrite_read()copies intocombinedfirst, so it is safe — but it only runs when there's a leftover tail from the previous chunk; the steady state hits the no-copy branch. buffer.ensure_still_alive()only roots the JS wrapper for GC; transfer/detach is orthogonal.- The Sink callbacks are documented as taking borrowed slices the embedder must copy before returning, but that protects the payload slice, not the outer
bytesslice the loop re-indexes after the callback.
Impact
User-reachable memory-safety issue: with a
createConnectionDuplex (used for proxies, HTTP/2-over-anything, and thetest-http2-generic-streams/backpressurefamily), a'ping'/'data'/'response'handler that detaches the chunk it was fed causes the native engine to read freed/overwritten memory on the very next frame in the same chunk — a use-after-free leading to garbage parsing or a crash. This is a regression of a previously-fixed bug whose regression test is removed by this same PR.Fix
Restore the pin around the borrow:
if let Some(array_buffer) = buffer.as_pinned_arraybuffer(global_object) { this.rewrite_read(array_buffer.byte_slice()); array_buffer.unpin(); Ok(JSValue::UNDEFINED) } else { ... }
(or, equivalently, make
rewrite_read()'s empty-tail branch copybytesinto an ownedVec<u8>before callingengine.receive(), matching what the non-empty-tail branch already does). And restore the deleted regression test.
187f68a to
f74f8c1
Compare
83618ad to
036fff3
Compare
e5dafc1 to
ada8836
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/http2.ts:4652-4662— The new CONNECT validation throws ($ERR_HTTP2_CONNECT_AUTHORITY/SCHEME/PATHat lines 4652-4661 and the array-path equivalents at ~4606-4608, plus the new$ERR_HTTP2_INVALID_SESSIONat 4547) all run beforegetNextStream(), but the pre-existingcatch (e) { this.#connections--; … }at line 4787 unconditionally decrements a counter that is only incremented insidegetNextStream()'sstreamStarthandler — sorequest({':method':'CONNECT'})(no:authority) throws and drives#connectionsto -1, breaking theif (this.#connections === 0)auto-destroy gate inclose()/streamEnd(andemitErrorNTalso fires a spurious session-level'error'for what Node treats as a pure synchronous user-input throw). The catch defect itself is pre-existing, but the CONNECT validation is the first realistic trigger this PR adds —test-http2-connect-method.js(ported by this PR) hits it three times →#connections = -3. Fix: track whethergetNextStream()ran (a local flag) and only decrement in the catch when it did.Extended reasoning...
What the bug is
ClientHttp2Session.request()atsrc/js/node/http2.ts:4542-4791wraps its body in atry/catchwhose catch block (lines 4787-4790) is:} catch (e: any) { this.#connections--; process.nextTick(emitErrorNT, this, e, this.#connections === 0 && this.#closed); throw e; }
#connectionsis incremented in exactly one place: thestreamStarthandler (line 3875,self.#connections++), which fires synchronously fromthis.#parser.getNextStream()at line 4756. Any throw before line 4756 reaches the catch with no matching increment, so the decrement underflows the counter.This PR adds several new throws before
getNextStream():$ERR_HTTP2_INVALID_SESSION()at line 4547 (new — destroyed sessions used to throwINVALID_STREAM, but it was the same shape).- The new object-path CONNECT validation at lines 4652-4661:
$ERR_HTTP2_CONNECT_AUTHORITY()/$ERR_HTTP2_CONNECT_SCHEME()/$ERR_HTTP2_CONNECT_PATH(). - The new array-path CONNECT validation at lines ~4606-4608 (same three errors).
The catch block itself is pre-existing (
git log -Ltraces it to the file's initial commit d6d8447), and pre-PR there were already throws beforegetNextStream()—$ERR_HTTP2_INVALID_STREAMon a destroyed/closed session,$ERR_INVALID_ARG_TYPEon non-object headers,$ERR_INVALID_ARG_VALUEon a badsensitivesarray — so the structural flaw is not new. But the CONNECT validation throws are new in this PR and are the first realistic trigger: forgetting:authorityon a CONNECT (or accidentally passing:path/:scheme) is a common user error that Node's API explicitly checks for, whereas the pre-existing triggers are programming errors that never reach a graceful-close gate anyway.Step-by-step proof
Take
test-http2-connect-method.js(ported by this PR), which on a connected client does:assert.throws(() => client.request({ ':method': 'CONNECT' }), { code: 'ERR_HTTP2_CONNECT_AUTHORITY' }); assert.throws(() => client.request({ ':method': 'CONNECT', ':authority': a, ':scheme': 'http' }), { code: 'ERR_HTTP2_CONNECT_SCHEME' }); assert.throws(() => client.request({ ':method': 'CONNECT', ':authority': a, ':path': '/' }), { code: 'ERR_HTTP2_CONNECT_PATH' });
Trace the first call:
request({':method':'CONNECT'})enters thetry. Not destroyed, not closed, nosentTrailers.headersis an object →headers = {...headers}.- Line 4652:
headers[':method'] === 'CONNECT' && headers[':protocol'] === undefined→ true. - Line 4653:
!headers[':authority']→!undefined→ true →throw $ERR_HTTP2_CONNECT_AUTHORITY(). getNextStream()(line 4756) never ran —streamStartnever fired —#connectionswas never incremented.- Catch at line 4787:
this.#connections--→ 0 → -1.process.nextTick(emitErrorNT, this, e, -1 === 0 && this.#closed)schedules a session-level error emission.throw ere-throws to the caller. assert.throwscatches the synchronous throw — the test passes that assertion.
After all three
assert.throwscalls,#connections === -3.Consequences
(a) Broken graceful-close gate. Both
ClientHttp2Session.close()(line 4475) and thestreamEndhandler checkthis.#connections === 0(andself.#connections === 0 && self.#closed) with strict equality, not<= 0. With the counter negative, those gates can never match at the right moment. Concretely:- After one bad CONNECT (
#connections = -1), the user opens one valid request →streamStart→#connections = 0. The user then callsclient.close():if (this.#connections === 0)is true with the request still in flight, sosetImmediate(destroy)fires and the in-flight request is torn down withERR_HTTP2_STREAM_CANCEL. - After three bad CONNECTs (
#connections = -3), each subsequent valid request increments to -2, -1, 0, … andstreamEnd's#connections === 0 && #closedauto-destroy lands at the wrong time or never.
In practice the socket-close path eventually destroys the session anyway (so this is not a hard hang), but the bookkeeping is unambiguously corrupted.
(b) Spurious session
'error'. Node treats CONNECT-validation failures as pure synchronous throws — they do not emit a session-level'error'event. HereemitErrorNT(line 391-401) fires on the next tick: it checksself.listenerCount('error') > 0first (line 398), so without a listener it's a silent no-op (which is whytest-http2-connect-method.jsdoesn't crash). But any user with a session'error'listener — e.g. for connection-loss handling — will see a confusing duplicate of the error they already caught synchronously.Why nothing prevents it
#connectionsis a plain private field with no clamp; the catch block has no flag to know whethergetNextStream()ran. The catch was clearly written assuming every throw happens after the increment, which was already false pre-PR and is now false for several more (and more reachable) cases.Why this is in scope for this PR
The catch flaw is pre-existing, but:
- CONNECT validation is a feature this PR explicitly adds (the PR description lists it under "Node compatibility fixes"), and that feature directly triggers the flaw.
- This PR ports
test-http2-connect-method.js, which exercises all three new throw paths on a live session. - The same fix also corrects the pre-existing trigger paths (
$ERR_INVALID_ARG_TYPEon non-object headers, etc.).
Fix
Track whether the increment actually happened and only undo it in the catch:
let streamStarted = false; try { … let stream_id: number = this.#parser.getNextStream(); streamStarted = true; … } catch (e: any) { if (streamStarted) { this.#connections--; process.nextTick(emitErrorNT, this, e, this.#connections === 0 && this.#closed); } throw e; }
(Dropping the
emitErrorNTfor the pre-getNextStream()case also fixes the spurious session-error: those throws are pure user-input validation that Node surfaces synchronously only.)
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/http2.ts:2622-2633— Nit: the newServerHttp2Stream._write()/_writev()overrides at lines 2622-2633 auto-respond()when!this.headersSentso DATA frames are preceded by response HEADERS, but there's no matching_final()override — soserver.on('stream', s => s.end())(no chunk, norespond()) goes straight to baseHttp2Stream._final()→native.writeStream(id, '', 'ascii', true, cb)at line 2371, putting an empty DATA|END_STREAM on the wire with no preceding response HEADERS. The PR fixed 2 of 3 outbound write paths for the same hazard the inline comment describes; add a_final(callback)override with the sameif (!this.headersSent && !this.destroyed && !this.closed) this.respond();guard beforesuper._final(callback).Extended reasoning...
What the bug is
This PR adds
ServerHttp2Stream._write()and_writev()overrides atsrc/js/node/http2.ts:2622-2633:// Node sends the implicit response headers (:status 200) when the stream is written to before // respond() was called; without this the DATA frames would go out with no preceding HEADERS. _write(chunk, encoding, callback) { if (!this.headersSent && !this.destroyed && !this.closed) { this.respond(); } super._write(chunk, encoding, callback); } _writev(data, callback) { if (!this.headersSent && !this.destroyed && !this.closed) { this.respond(); } super._writev(data, callback); }
The inline comment (lines 2620-2621) is explicit: "without this the DATA frames would go out with no preceding HEADERS". But there is no
ServerHttp2Stream._final()override (a grep confirms only one_final(callback)definition in the file, at line 2327 on the baseHttp2Stream), and the baseHttp2Stream._final()at line 2371 also writes a DATA frame:native.writeStream(this.#id, "", "ascii", true, callback);
— an empty DATA frame with
close=true(END_STREAM). So the third outbound-write path has the exact hazard the first two were just fixed for.The code path that triggers it
Writable.prototype.end()with no chunk does not call_write()— it goesprefinish→_final(callback)directly.Http2Stream.end()'s own internal comment in this file confirms this: "just calls _final without _write for empty data". So a server'stream'handler that callsstream.end()without first callingstream.respond()(and without writing any data) bypasses the auto-respond entirely.Step-by-step proof
Take
server.on('stream', s => s.end()):- The client's request HEADERS arrives → engine
on_stream_open→ JS createsServerHttp2Stream(1)and emits'stream'. - The handler calls
s.end()with no chunk.headersSentisfalse(respond()never called). Duplex.prototype.end()→ no chunk →_write()is not invoked. The Writable side moves toprefinish→_final(callback).ServerHttp2Streamhas no_finaloverride, so baseHttp2Stream._final()(line 2327) runs.pendingis false (the stream has an id).waitForTrailersis unset.- Line 2371:
native.writeStream(1, "", "ascii", true, callback)enqueues a DATA frame with END_STREAM on stream 1. - The legacy
Stream::can_send_data()check returns true forHALF_CLOSED_REMOTE, so the frame is written to the socket: a 9-byteDATA(stream=1, flags=END_STREAM, length=0). - No response HEADERS frame was ever sent on stream 1. Per RFC 9113 §8.3.2, every response must contain a
:statuspseudo-header in a HEADERS frame; sending DATA before that is a malformed response. A compliant client (nghttp2/Node) typically resets the stream with PROTOCOL_ERROR.
Had the handler instead called
s.end('x')ors.write('x'); s.end(), step 3 would have routed throughServerHttp2Stream._write('x', …), which auto-respond()s first — so the asymmetry is purely the bare-end()case.Why nothing prevents it
The auto-respond guard lives only in the two new
_write/_writevoverrides. Nothing in base_final()checksheadersSent, and nothing else on the server side interceptsend()-with-no-chunk. Pre-PR there was no auto-respond on any of the three paths, so this isn't a regression — the PR added a new feature to two of the three sibling write hooks and missed the third.A note on Node parity: Node's own
_final()does not call[kProceed](auto-respond) either — only Node's_write/_writevdo. But Node's_final()callshandle.shutdown()→nghttp2_session_resume_data(), which for a stream with no submitted response is a no-op (nghttp2 has nothing to resume) and sends nothing on the wire. Bun's_final()actively writes a DATA frame vianative.writeStream. So Bun diverges from Node (Bun sends a malformed DATA; Node sends nothing), and given Bun's architecture the consistent fix is to apply the same auto-respond guard the PR already added to_write/_writev.Impact
Nit. Narrow trigger: a server handler must call bare
stream.end()with neither a priorwrite()nor an explicitrespond()— uncommon, since most code either callsrespond()orend(data). Pre-PR none of the three paths auto-responded, so this is an incomplete new feature rather than a regression. The fix is one method.Fix
Add the same guard as a
_finaloverride onServerHttp2Stream, alongside the_write/_writevoverrides:_final(callback) { if (!this.headersSent && !this.destroyed && !this.closed) { this.respond(); } super._final(callback); }
- The client's request HEADERS arrives → engine
cirospaciari
changed the title
cirospaciari
changed the title
2 participants
Summary
Rewrites the inbound half of
node:http2on a self-contained HTTP/2 frame engine, closes a large set of Node compatibility gaps, and overhauls the outbound write path end to end (frame batching, vectored writes, TLS record batching). The ported test suite is byte-identical to node v26.3.0 — every deviation from upstream behavior is either fixed in the implementation or registered explicitly intest/expectations.txt.src/runtime/api/bun/h2/: wire codec, SETTINGS, flow control, stream state machine, HPACK over lshpack, connection core). All inbound traffic — native sockets and JS-fed streams — runs through it; outbound still uses the existing encoder (follow-up).pushStream/createPushResponse, pushed-stream lifecycle,maxReservedRemoteStreams, push-disabled enforcement (RFC 9113 §6.6).maxSettings/maxOutstandingSettings, §6.9 flow-control windows including the pre-ACK initial-window rule (§6.5.3) and the §6.9.2 delta, frame-size limits enforced at header-parse time, CONTINUATION sequencing as connection errors (§6.2/§6.10), closed-stream eviction,initialWindowSizebounded at 2^31-1 (§6.5.2).:authoritymatching node'skAuthority, GOAWAY semantics (ERR_HTTP2_GOAWAY_SESSIONon new requests, REFUSED_STREAM sweep of unprocessed streams — grpc fails over correctly), session destroy semantics,respondWithFDaccepting FileHandle, node-exact error messages on the http2 surface,writeInformation,diagnostics_channelintegration, AltSvc/Origin frames, extended CONNECT (:protocol),allowHTTP1fallback, graceful close with correct GOAWAY last-stream-id.src/jsc/bindings/H2HeadersMaterializer.cpp): the raw headers array and the node-shaped headers object (fulltoHeaderObjectsemantics — set-cookie arrays, cookie joins, single-value first-wins,:statuscoercion, sensitive-headers symbol) are built together in C++, reusing WebCore's interned header-name strings.writevwith payload slices referenced zero-copy. Flush paths never hold thread-local borrows across socket writes (h2-over-h2 tunnels re-enter the writer through JS-stream-backed sockets).packages/bun-usockets, benefits all TLS serving, not just h2): sealed records accumulate and reach the socket in 128 KB batches instead of one write per 16 KB record, with honest write accounting — consumption stops when the wire blocks, a bounded spill drains before any graceful shutdown, so flush-then-close semantics (destroySoon) hold. Newbsd_writev/us_socket_raw_writevprimitives (POSIXwritev, sequential fallback elsewhere).--expose-internalsrun via virtualinternal/http2/*modules backed by the real internals.Test results
h2-conformance.test.ts)test-tls-*parallel suite (after the uSockets changes)Remaining failures are tracked in
test/expectations.txtwith reasons: node-internals-dependent tests (internalBindingmonkey-patching), observability (NODE_DEBUG trace, perf_hooks entries, AsyncLocalStorage propagation), validator wording (andvs&&in the shared range template),customSettings, the nghttp2 flow-control error surface, paddingStrategy, and platform-specific teardown gaps.Follow-ups (tracked in review threads)
Performance
http2.createServer/createSecureServerhello + streaming servers,oha --http-version 2, macOS arm64 release build (representative runs; local builds vary a few percent):bench/grpc-server(grpc-js unary ping,ghz --total=60000 -c 50):What gets the rewrite there:
writev: frame headers from a small scratch plus payload slices straight from the caller's buffer — no per-frame copy, one syscall per body.packages/bun-usockets): oneSSL_writeconsumes plaintext in record-size slices, ciphertext accumulates and hits the socket in 128 KB batches instead of one write per 16 KB record. Write accounting stays honest — when the wire blocks, consumption stops, so flush-then-close semantics (destroySoon) hold; a bounded spill drains before any graceful shutdown.rstStreamfor cleanly closed streams; local-close state rides thewriteStreamreturn value; header objects never flip into dictionary mode.