node:http2: fix request() option validation ordering, parent: 0, and the PADDED+PRIORITY HEADERS layout

Closed
RO
2mo ago
+404-211

What

An http2.connect() client request carrying priority options could reject the stream with NGHTTP2_INTERNAL_ERROR, and as a side effect corrupt the session's HPACK encoder state so that the next, perfectly normal request on the same connection was killed by the peer with a connection-level GOAWAY(COMPRESSION_ERROR). One bad request poisoned the whole session.

A(weight:7)         OK 200
B(excl+parent:0)    ERROR ERR_HTTP2_STREAM_ERROR Stream closed with error code NGHTTP2_INTERNAL_ERROR
C(exclusive only)   ERROR ERR_HTTP2_STREAM_ERROR Stream closed with error code NGHTTP2_PROTOCOL_ERROR
D(plain)            ERROR ERR_HTTP2_STREAM_ERROR Stream closed with error code NGHTTP2_PROTOCOL_ERROR

Isolated per option shape against a node v26 http2 server, each on a fresh session followed by a plain request:

request optionsnode clientbun client (before)
{ parent: 0 }200, 200NGHTTP2_INTERNAL_ERROR, then plain request dies with server GOAWAY(COMPRESSION_ERROR)
{ weight: 0 }200, 200same
{ weight: 256 }200, 200same
session paddingStrategy: MAX + { exclusive: true }200, 200connection torn down (see the second fix below)
{ weight: 7 } / { exclusive: true } / { parent: 3 }200, 200200, 200

Cause

1. Options were validated after the header block was already fed to the shared HPACK encoder.

H2FrameParser.request() encoded the header block into the session's shared HPACK encoder first, and only then read and validated the options object. Every rejection in the options path (!options.is_object(), parent <= 0, weight < 1 || weight > 255, a non-boolean option's ERR_INVALID_ARG_TYPE, an already-aborted signal, the maxSessionMemory ENHANCE_YOUR_CALM check) returned without writing the HEADERS frame.

encode_header_into_list inserts entries into the encoder's dynamic table (lshpack) as it goes. Once the block is encoded, the encoder is committed: the peer's decoder learns those inserts only from the frame that was never sent. The next header block on the session references dynamic-table indices the server does not have, the server's HPACK decode fails, and it tears the connection down with COMPRESSION_ERROR.

The trigger people actually hit is parent: 0. Node accepts it: 0 is the connection stream, the RFC 7540 section 5.3.1 root, and is exactly what node's own setAndValidatePriorityOptions fills in when parent is unset. Bun's check was parent <= 0. weight: 0 / weight: 256 (both legal in node, which forwards any numeric weight and relies on nghttp2's adjust_weight clamp) hit the same post-encode rejection.

2. A HEADERS frame carrying both PADDED and PRIORITY had the wrong payload layout.

RFC 7540 section 6.2 puts the Pad Length octet first in the payload, before the optional priority fields. request() wrote the 5 priority bytes first. A peer parsing per the RFC reads the high byte of the stream-dependency word as the pad length, misaligns the header block, and tears the whole connection down. Bun's own receive side (handle_headers_frame) already reads Pad Length first, so a bun client poisoned a bun server too. Any session with paddingStrategy: PADDING_STRATEGY_MAX or ALIGNED hit this on every priority-optioned request.

Fix

src/runtime/api/bun/h2_frame_parser.rs:

  • All of options (and the session memory limit) are now parsed and validated at the top of request(), before any header is fed to the encoder. The parsed values are applied to the stream after the encode. No options-path rejection can desync the encoder anymore; rejections that need a registered stream go through a new reject_request_stream helper.
  • parent: 0 is accepted (parent < 0 is still rejected).
  • weight is clamped to the encodable range instead of erroring, matching nghttp2.
  • The single-frame HEADERS writer now emits the Pad Length octet before the priority fields.
  • Reordering the option parsing separated the signal's aborted() check from attach_signal() by the encode loops, which can run arbitrary user JS (a header value's toString). addListener on an already-aborted signal fires its callback synchronously, so an abort raced in during the encode would have written RST_STREAM for a stream the peer has never seen and re-entered the Stream that request() holds &mut to, before request() then wrote the HEADERS for the same id. aborted() is re-checked at the attach site; if the abort raced in, the HEADERS frame is written (the block is already committed to the encoder) and only then is the stream aborted, the same wire shape as an abort that arrives one tick after request() returns.

Intentionally excluded, same bug class, different fix:

  • A header name/value validation failure in the middle of the encode loop (for example a NUL byte in the value of the third header, after the first two were already encoded) still desyncs the encoder through the same mechanism. Fixing that needs the request header loops split into a validate pass and an encode pass (or the encoder snapshot/rolled back), which is a larger restructure than the options hoist; left for a follow-up.
  • The maxSendHeaderBlockLength rejection still runs after the encode because it measures the encoded block size; nghttp2 terminates the whole session in that case too, and it only applies when the session explicitly opts into that limit.

Test

test/js/node/http2/node-http2.test.js, three tests sharing one helper:

  • one session issues a request for each priority-option shape node accepts, then a plain request. The plain request is the load-bearing assertion: it proves none of the optioned requests left the encoder mid-block.
  • a session with paddingStrategy: MAX / ALIGNED issues a priority-optioned request, then a plain request.
  • a request whose header value's toString aborts its own signal mid-encode errors with an AbortError and leaves the session usable for a plain follow-up.

All three fail on the unfixed build with the reported errors:

error: Stream closed with error code NGHTTP2_INTERNAL_ERROR
 code: "ERR_HTTP2_STREAM_ERROR"
(fail) request() priority options are accepted and never desync the session's HPACK encoder
error: Stream closed with error code NGHTTP2_COMPRESSION_ERROR
(fail) HEADERS carrying both PADDED and PRIORITY puts Pad Length before the priority fields
error: New streams cannot be created after receiving a GOAWAY
(fail) a signal aborted from a header value's toString during request() does not poison the session

All pass with the fix, and test/js/node/http2/node-http2.test.js plus the h2 conformance / padding / continuation suites pass.

Standalone repro (works against a node http2 server or a bun one)
const http2 = require("node:http2");
const server = http2.createServer();
server.on("stream", (s, h) => { s.respond({ ":status": 200 }); s.end("ok"); });
server.listen(0, () => {
  const client = http2.connect(`http://localhost:${server.address().port}`);
  client.on("goaway", code => console.log("goaway", code)); // 9 = COMPRESSION_ERROR
  const go = (p, o) => new Promise(r => {
    const q = o ? client.request({ ":path": p }, o) : client.request({ ":path": p });
    q.resume();
    q.on("error", e => (console.log(p, e.code), r()));
    q.on("close", () => (console.log(p, "ok"), r()));
    q.end();
  });
  (async () => {
    await go("/a", { parent: 0 }); // node-valid; bun: NGHTTP2_INTERNAL_ERROR
    await go("/b");                // collateral: server GOAWAYs with COMPRESSION_ERROR
    process.exit(0);
  })();
});

Rebase note

Rebased past #33072, whose enter_stream_dispatch/GuardedStream guard lands at the same request() site this PR rewrites. Both changes are kept: this PR's options-before-encode ordering (the HPACK desync fix) plus #33072's dispatch guard at the post-encode &mut Stream borrow. The two new pre-encode sites that hold a &mut Stream across a JS dispatch (the reject_request_stream helper and the already-aborted-signal fast path) now use enter_stream_dispatch as well.


[review] gate passed · iteration 5 · 2 files touched

fails on main (without fix)
ASAN without fix: 4 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (05a902f8f)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [725.85ms]
(pass) node none > Client Basics > should be able to send a POST request [506.00ms]
(pass) node none > Client Basics > constants [18.42ms]
(pass) node none > Client Basics > getDefaultSettings [7.14ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [23.15ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [5.54ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.33ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.20ms
... (truncated)

release without fix: 9 failed, 6 skipped
bun test v1.4.0-canary.1 (1498d7b77)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [1.19ms]
(pass) node none > Client Basics > getDefaultSettings [0.25ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.71ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.15ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.11ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.15ms]
(pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [2.75ms]
(pass) node none > Client Basics > is possible to abort request [1.52ms]
(pass) node none > Client Basics > aborted event should work with abortController [1.05ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.87ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.46ms]
(skip) node none > Client Basics > should not leak memory
(pass) node none > Client Basics > close callback [25.
... (truncated)
passes on PR (with fix)
ASAN with fix: 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (05a902f8f)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [722.75ms]
(pass) node none > Client Basics > should be able to send a POST request [493.95ms]
(pass) node none > Client Basics > constants [20.04ms]
(pass) node none > Client Basics > getDefaultSettings [7.01ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.49ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [5.35ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.14ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.46ms
... (truncated)

release with fix: 6 skipped
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     05a902f8f4
  features     (none)

22 deps, 105 codegen, 1167 objects in 771ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1230] install /workspace/bun
bun install v1.4.0-canary.1 (1498d7b77)

Checked 124 installs across 170 packages (no changes) [8.00ms]
[2/1230] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (1498d7b77)

Checked 1 install across 2 packages (no changes) [2.00ms]
[3/1230] gen bindgenv2
[4/1230] gen ErrorCode+*.h
[5/1230] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (1498d7b77)

Checked 129 installs across 147 packages (no changes) [7.00ms]
[6/1230] fetch tinycc
[tinycc] up to date
[7/1229] fetch picohttpparser
[picohttpparser] up to date
[8/1229] fetch l
... (truncated)
diff hotspot
src/runtime/api/bun/h2_frame_parser.rs | 469 ++++++++++++++++++---------------
 test/js/node/http2/node-http2.test.js  | 146 ++++++++++
 2 files changed, 404 insertions(+), 211 deletions(-)

gate history · 1 passed · 0 rejected · iteration 5

evidence per changed file
file                                    reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs     21     19     20
test/js/node/http2/node-http2.test.js       2      6     19