diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index 1be650bff0d6..fa68ff122890 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -8271,23 +8271,23 @@ impl H2FrameParser { Ok(JSValue::js_number(result as f64)) } - /// `set_next_stream_id` can park `last_stream_id` anywhere in the u32 range, so the step - /// saturates; callers that open the stream reject anything above `MAX_STREAM_ID`. + /// First id of this side's parity above `last_stream_id`, which never exceeds `MAX_STREAM_ID`. fn get_next_stream_id(&self) -> u32 { let stream_id = self.last_stream_id.get(); if self.is_server.get() { if stream_id.is_multiple_of(2) { - stream_id.saturating_add(2) + stream_id + 2 } else { - stream_id.saturating_add(1) + stream_id + 1 } } else if stream_id.is_multiple_of(2) { - stream_id.saturating_add(1) + stream_id + 1 } else { - stream_id.saturating_add(2) + stream_id + 2 } } + /// Node's setNextStreamID is `nghttp2_session_set_next_stream_id`: invalid ids are ignored. #[bun_jsc::host_fn(method)] pub(crate) fn set_next_stream_id( this: &Self, @@ -8298,22 +8298,16 @@ impl H2FrameParser { debug_assert!(args_list.len() >= 1); let stream_id_arg = args_list[0]; debug_assert!(stream_id_arg.is_number()); - // Store the id `get_next_stream_id` steps from. A fractional id passes the JS layer's - // `id <= 0` check and truncates to 0 here; 0 (and 1 on a client) has no predecessor, - // so the subtraction saturates to the initial state instead of wrapping. let next_stream_id = stream_id_arg.to_u32(); - let last_stream_id = if this.is_server.get() { - if next_stream_id.is_multiple_of(2) { - next_stream_id.saturating_sub(2) - } else { - next_stream_id.saturating_sub(1) - } - } else if next_stream_id.is_multiple_of(2) { - next_stream_id.saturating_sub(1) - } else { - next_stream_id.saturating_sub(2) - }; - this.last_stream_id.set(last_stream_id); + let local_parity = u32::from(!this.is_server.get()); + if next_stream_id % 2 != local_parity + || next_stream_id > MAX_STREAM_ID + || next_stream_id <= this.get_next_stream_id() + { + return Ok(JSValue::UNDEFINED); + } + // Above the next id with this side's parity, so this still moves `last_stream_id` forward. + this.last_stream_id.set(next_stream_id - 2); Ok(JSValue::UNDEFINED) } diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 26f4f42a1597..411ec9ec3510 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -2671,8 +2671,9 @@ it("http2 client.setNextStreamID validates input", async () => { it("http2 setNextStreamID at the edges of the id space does not overflow", async () => { // 0.5 passes the JS range check (> 0) and reaches the native setter as 0, which used to - // underflow (node ends up on stream 1 too). A server parked at 2 ** 32 - 1 used to overflow - // when the next id was computed; it has to read back as an unusable id, not wrap to a low one. + // underflow (node ends up on stream 1 too). 2 ** 32 - 1 passes the JS range check as well; on a + // server it is ignored as an odd id (node ignores it too), where it used to be stored and + // overflow the next id computation. The 31-bit bound itself is covered by the next test. const fixture = ` const http2 = require("node:http2"); const result = { client: {}, server: {} }; @@ -2724,12 +2725,153 @@ it("http2 setNextStreamID at the edges of the id space does not overflow", async expect(stderr).toBe(""); expect(JSON.parse(stdout.trim())).toEqual({ client: { 0.5: 1, 1: 1 }, - server: { 0: 2, 2: 2, 4294967295: 4294967295 }, + server: { 0: 2, 2: 2, 4294967295: 2 }, response: { streamId: 1, status: 200 }, }); expect(exitCode).toBe(0); }); +it("http2 setNextStreamID ignores the ids nghttp2 rejects instead of rounding them or moving backwards", async () => { + // Expected values come from node v26.3.0, where setNextStreamID ends up in + // nghttp2_session_set_next_stream_id: an id of the peer's parity, an id that is not above the + // current next id, or one that does not fit in 31 bits leaves the session untouched. The server + // side is checked while handling the client's first stream (id 1), where node reports 2 as the + // server's next id too. The "equal to next" rows also read lastProcStreamID: it is the one value + // that tells an ignored call apart from one that re-stores the current position. + const readState = ({ nextStreamID, lastProcStreamID }) => [nextStreamID, lastProcStreamID]; + const server = http2.createServer(); + const serverSide = Promise.withResolvers(); + const pushed = Promise.withResolvers(); + const failure = Promise.withResolvers(); + server.on("sessionError", failure.reject); + server.on("stream", (stream, headers) => { + if (headers[":path"] !== "/first") { + stream.respond(); + stream.end(); + return; + } + try { + const { session } = stream; + // ServerHttp2Session does not expose setNextStreamID; call the native setter it would wrap. + const parser = session[Symbol.for("::bunhttp2native::")]; + const set = id => { + parser.setNextStreamID(id); + return session.state.nextStreamID; + }; + const seen = { "initial [next, lastProc]": readState(session.state) }; + parser.setNextStreamID(2); + seen["set 2 (equal to next) [next, lastProc]"] = readState(session.state); + seen["set 100"] = set(100); + seen["set 101 (odd)"] = set(101); + seen["set 105 (odd)"] = set(105); + seen["set 50 (below next)"] = set(50); + seen["set 100 (equal to next)"] = set(100); + stream.pushStream({ ":path": "/pushed" }, (err, pushedStream) => { + if (err) return failure.reject(err); + pushedStream.respond(); + pushedStream.end(); + }); + seen["after pushStream"] = session.state.nextStreamID; + seen["set 50 after pushStream"] = set(50); + seen["set 104 (lowest id above next)"] = set(104); + seen["set 2 ** 31 (not 31-bit)"] = set(2 ** 31); + seen["set 2 ** 32 - 2 (not 31-bit)"] = set(2 ** 32 - 2); + seen["set 2 ** 31 - 2 (highest even)"] = set(2 ** 31 - 2); + serverSide.resolve(seen); + } catch (err) { + failure.reject(err); + } + stream.respond(); + stream.end(); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const client = http2.connect(`http://127.0.0.1:${server.address().port}`); + client.on("error", failure.reject); + client.once("stream", (pushedStream, pushHeaders) => { + pushedStream.resume(); + pushed.resolve({ id: pushedStream.id, path: pushHeaders[":path"] }); + }); + const set = id => { + client.setNextStreamID(id); + return client.state.nextStreamID; + }; + const request = path => + new Promise((resolve, reject) => { + const req = client.request({ ":path": path }); + let status; + req.on("error", reject); + req.on("response", responseHeaders => (status = responseHeaders[":status"])); + req.on("close", () => resolve({ id: req.id, status })); + req.resume(); + req.end(); + }); + const run = async () => { + await new Promise(resolve => client.once("connect", resolve)); + const seen = { "initial [next, lastProc]": readState(client.state) }; + client.setNextStreamID(1); + seen["set 1 (equal to next) [next, lastProc]"] = readState(client.state); + seen["request /first"] = await request("/first"); + seen["set 5 (lowest id above next)"] = set(5); + seen["request /second"] = await request("/second"); + seen["set 11"] = set(11); + seen["set 12 (even)"] = set(12); + seen["set 14 (even)"] = set(14); + seen["set 7 (below next)"] = set(7); + seen["set 11 (equal to next)"] = set(11); + seen["request /third"] = await request("/third"); + seen["after /third"] = client.state.nextStreamID; + seen["set 7 after /third"] = set(7); + seen["request /fourth"] = await request("/fourth"); + seen["set 2 ** 31 + 1 (not 31-bit)"] = set(2 ** 31 + 1); + seen["set 2 ** 32 - 1 (not 31-bit)"] = set(2 ** 32 - 1); + seen["set 2 ** 31 - 1 (highest odd)"] = set(2 ** 31 - 1); + return { client: seen, server: await serverSide.promise, pushed: await pushed.promise }; + }; + + try { + expect(await Promise.race([run(), failure.promise])).toEqual({ + client: { + "initial [next, lastProc]": [1, 0], + "set 1 (equal to next) [next, lastProc]": [1, 0], + "request /first": { id: 1, status: 200 }, + "set 5 (lowest id above next)": 5, + "request /second": { id: 5, status: 200 }, + "set 11": 11, + "set 12 (even)": 11, + "set 14 (even)": 11, + "set 7 (below next)": 11, + "set 11 (equal to next)": 11, + "request /third": { id: 11, status: 200 }, + "after /third": 13, + "set 7 after /third": 13, + "request /fourth": { id: 13, status: 200 }, + "set 2 ** 31 + 1 (not 31-bit)": 15, + "set 2 ** 32 - 1 (not 31-bit)": 15, + "set 2 ** 31 - 1 (highest odd)": 2 ** 31 - 1, + }, + server: { + "initial [next, lastProc]": [2, 1], + "set 2 (equal to next) [next, lastProc]": [2, 1], + "set 100": 100, + "set 101 (odd)": 100, + "set 105 (odd)": 100, + "set 50 (below next)": 100, + "set 100 (equal to next)": 100, + "after pushStream": 102, + "set 50 after pushStream": 102, + "set 104 (lowest id above next)": 104, + "set 2 ** 31 (not 31-bit)": 104, + "set 2 ** 32 - 2 (not 31-bit)": 104, + "set 2 ** 31 - 2 (highest even)": 2 ** 31 - 2, + }, + pushed: { id: 100, path: "/pushed" }, + }); + } finally { + client.close(); + server.close(); + } +}); + it("http2 request.destroy() with error", async () => { const server = http2.createServer();