Skip to content

Commit 0eebafb

Browse files
mcollinaaduh95
authored andcommitted
http2: release headers when a stream is removed
When a peer sends more header fields than `maxHeaderListPairs`, the stream is reset and the fields it had already accepted stay in `current_headers_`. Each of them holds nghttp2 buffers. `RemoveStream()` adjusted the session memory accounting but kept the headers. A destroyed stream that still has a write on the socket is not detached and can be collected after its session. The session destructor only detaches the streams it still knows about, so the nghttp2 memory held by the removed stream is still allocated and `CHECK_EQ(current_nghttp2_memory_, 0)` aborts the process. A client can trigger this with a single connection. Clear the headers in `RemoveStream()`, while the session is still alive, and in the session destructor for the streams that are still registered, since `Detach()` does not free a stream that is referenced elsewhere. Refs: https://hackerone.com/reports/4150893 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#1026 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Tim Perry <pimterry@gmail.com> CVE-ID: CVE-2026-123863
1 parent d5a5280 commit 0eebafb

2 files changed

Lines changed: 95 additions & 0 deletions

File tree

‎src/node_http2.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,9 @@ Http2Session::~Http2Session() {
640640
// Ensure that all `Http2Stream` instances and the memory they hold
641641
// on to are destroyed before the nghttp2 session is.
642642
for (const auto& [id, stream] : streams_) {
643+
// Detach() only frees the stream if nothing else holds a strong
644+
// reference to it.
645+
stream->ClearHeaders();
643646
stream->Detach();
644647
}
645648
streams_.clear();
@@ -949,6 +952,11 @@ BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
949952
stream = FindStream(id);
950953
if (stream) {
951954
streams_.erase(id);
955+
// The stream can outlive the session, for example while one of its writes
956+
// is still on the socket. Headers that were not passed to JS, such as an
957+
// oversized block that caused the stream to be reset, hold nghttp2 memory
958+
// that has to be released before the session is.
959+
stream->ClearHeaders();
952960
if (stream->current_headers_length_ > 0) {
953961
DecrementCurrentSessionMemory(stream->current_headers_length_);
954962
stream->current_headers_length_ = 0;
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// Flags: --expose-gc
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
// A stream that is reset because of an oversized header block keeps the
9+
// headers it had already accepted. If the stream is destroyed while one of
10+
// its writes is still on the socket, it outlives the session, and the session
11+
// destructor used to abort because that memory was still allocated.
12+
13+
const http2 = require('http2');
14+
const net = require('net');
15+
const { setTimeout: sleep } = require('timers/promises');
16+
17+
// Enough response data to fill the socket buffers on loopback.
18+
const kStreams = 80;
19+
20+
function frame(type, flags, id, payload = Buffer.alloc(0)) {
21+
const header = Buffer.alloc(9);
22+
header.writeUIntBE(payload.length, 0, 3);
23+
header[3] = type;
24+
header[4] = flags;
25+
header.writeUInt32BE(id, 5);
26+
return Buffer.concat([header, payload]);
27+
}
28+
29+
// :method POST, :scheme http, :path /, :authority localhost
30+
const requestHeaders = Buffer.from([
31+
0x83, 0x86, 0x84, 0x01, 0x09, ...Buffer.from('localhost'),
32+
]);
33+
34+
// More trailer fields than the default maxHeaderListPairs (128).
35+
const trailers = Buffer.concat(Array.from({ length: 200 }, (_, i) => {
36+
const name = `x-${i}`;
37+
return Buffer.from([0x00, name.length, ...Buffer.from(name), 0x01, 0x76]);
38+
}));
39+
40+
const server = http2.createServer(common.mustCall((req, res) => {
41+
req.on('error', () => {});
42+
res.on('error', () => {});
43+
// Respond before the request has ended, with more data than the client
44+
// reads, so that the response stays in a pending socket write.
45+
res.end(Buffer.alloc(100 * 1024));
46+
}, kStreams));
47+
48+
server.on('session', common.mustCall((session) => {
49+
session.on('error', () => {});
50+
session.on('close', common.mustCall(async () => {
51+
server.close();
52+
// Collect the session before the streams that are still referenced by
53+
// their pending writes.
54+
for (let i = 0; i < 10; i++) {
55+
globalThis.gc();
56+
await sleep(10);
57+
}
58+
}));
59+
}));
60+
61+
server.listen(0, common.mustCall(async () => {
62+
const client = net.connect(server.address().port);
63+
client.on('error', () => {});
64+
client.pause();
65+
66+
const settings = Buffer.from([0x00, 0x04, 0x7f, 0xff, 0xff, 0xff]);
67+
const windowUpdate = Buffer.alloc(4);
68+
windowUpdate.writeUInt32BE(0x7fffffff - 65535);
69+
const requests = [];
70+
for (let i = 0; i < kStreams; i++)
71+
requests.push(frame(1, 0x4, 1 + 2 * i, requestHeaders));
72+
client.write(Buffer.concat([
73+
Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'),
74+
frame(4, 0, 0, settings),
75+
frame(8, 0, 0, windowUpdate),
76+
...requests,
77+
]));
78+
await sleep(common.platformTimeout(500));
79+
80+
const blocks = [];
81+
for (let i = 0; i < kStreams; i++)
82+
blocks.push(frame(1, 0x5, 1 + 2 * i, trailers));
83+
client.write(Buffer.concat(blocks));
84+
await sleep(common.platformTimeout(200));
85+
86+
client.resetAndDestroy();
87+
}));

0 commit comments

Comments
 (0)