src: fix HTTP2 mem leak on premature close and ERR_PROTO · nodejs/node@984f735 · GitHub
Skip to content

Commit 984f735

Browse files
RafaelGSSRafaelGSS
committed
src: fix HTTP2 mem leak on premature close and ERR_PROTO
This commit fixes a memory leak when the socket is suddenly closed by the peer (without GOAWAY notification) and when invalid header (by nghttp2) is identified and the connection is terminated by peer. Refs: https://hackerone.com/reports/2841362 PR-URL: nodejs-private/node-private#650 Reviewed-By: James M Snell <jasnell@gmail.com> CVE-ID: CVE-2025-23085
1 parent 2446870 commit 984f735

7 files changed

Lines changed: 220 additions & 10 deletions

‎lib/internal/http2/core.js‎

Lines changed: 12 additions & 3 deletions

‎src/node_http2.cc‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -857,6 +857,7 @@ bool Http2Session::CanAddStream() {
857857
}
858858

859859
void Http2Session::AddStream(Http2Stream* stream) {
860+
Debug(this, "Adding stream: %d", stream->id());
860861
CHECK_GE(++statistics_.stream_count, 0);
861862
streams_[stream->id()] = BaseObjectPtr<Http2Stream>(stream);
862863
size_t size = streams_.size();
@@ -867,6 +868,7 @@ void Http2Session::AddStream(Http2Stream* stream) {
867868

868869

869870
BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
871+
Debug(this, "Removing stream: %d", id);
870872
BaseObjectPtr<Http2Stream> stream;
871873
if (streams_.empty())
872874
return stream;
@@ -1043,6 +1045,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle,
10431045
if (!stream) [[unlikely]]
10441046
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
10451047

1048+
Debug(session, "handling header key/pair for stream %d", id);
10461049
// If the stream has already been destroyed, ignore.
10471050
if (!stream->is_destroyed() && !stream->AddHeader(name, value, flags)) {
10481051
// This will only happen if the connected peer sends us more
@@ -1112,9 +1115,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
11121115
return 1;
11131116
}
11141117

1115-
// If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
1118+
// If the error is fatal or if error code is one of the following
1119+
// we emit and error:
1120+
//
1121+
// ERR_STREAM_CLOSED: An invalid frame has been received in a closed stream.
1122+
//
1123+
// ERR_PROTO: The RFC 7540 specifies:
1124+
// "An endpoint that encounters a connection error SHOULD first send a GOAWAY
1125+
// frame (Section 6.8) with the stream identifier of the last stream that it
1126+
// successfully received from its peer.
1127+
// The GOAWAY frame includes an error code that indicates the type of error"
1128+
// The GOAWAY frame is already sent by nghttp2. We emit the error
1129+
// to liberate the Http2Session to destroy.
11161130
if (nghttp2_is_fatal(lib_error_code) ||
1117-
lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) {
1131+
lib_error_code == NGHTTP2_ERR_STREAM_CLOSED ||
1132+
lib_error_code == NGHTTP2_ERR_PROTO) {
11181133
Environment* env = session->env();
11191134
Isolate* isolate = env->isolate();
11201135
HandleScope scope(isolate);
@@ -1177,7 +1192,6 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
11771192
Debug(session, "frame type %d was not sent, code: %d",
11781193
frame->hd.type, error_code);
11791194

1180-
// Do not report if the frame was not sent due to the session closing
11811195
if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
11821196
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
11831197
error_code == NGHTTP2_ERR_STREAM_CLOSING) {
@@ -1186,7 +1200,15 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
11861200
// to destroy the session completely.
11871201
// Further information see: https://github.com/nodejs/node/issues/35233
11881202
session->DecrefHeaders(frame);
1189-
return 0;
1203+
// Currently, nghttp2 doesn't not inform us when is the best
1204+
// time to call session.close(). It relies on a closing connection
1205+
// from peer. If that doesn't happen, the nghttp2_session will be
1206+
// closed but the Http2Session will still be up causing a memory leak.
1207+
// Therefore, if the GOAWAY frame couldn't be send due to
1208+
// ERR_SESSION_CLOSING we should force close from our side.
1209+
if (frame->hd.type != 0x03) {
1210+
return 0;
1211+
}
11901212
}
11911213

11921214
Isolate* isolate = env->isolate();
@@ -1252,12 +1274,15 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
12521274
// ignore these. If this callback was not provided, nghttp2 would handle
12531275
// invalid headers strictly and would shut down the stream. We are intentionally
12541276
// being more lenient here although we may want to revisit this choice later.
1255-
int Http2Session::OnInvalidHeader(nghttp2_session* session,
1277+
int Http2Session::OnInvalidHeader(nghttp2_session* handle,
12561278
const nghttp2_frame* frame,
12571279
nghttp2_rcbuf* name,
12581280
nghttp2_rcbuf* value,
12591281
uint8_t flags,
12601282
void* user_data) {
1283+
Http2Session* session = static_cast<Http2Session*>(user_data);
1284+
int32_t id = GetFrameID(frame);
1285+
Debug(session, "invalid header received for stream %d", id);
12611286
// Ignore invalid header fields by default.
12621287
return 0;
12631288
}
@@ -1653,6 +1678,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
16531678

16541679
// Called by OnFrameReceived when a complete SETTINGS frame has been received.
16551680
void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
1681+
Debug(this, "handling settings frame");
16561682
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
16571683
if (!ack) {
16581684
js_fields_->bitfield &= ~(1 << kSessionRemoteSettingsIsUpToDate);

‎test/parallel/test-http2-connect-method-extended-cant-turn-off.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,10 @@ server.listen(0, common.mustCall(() => {
2727
server.close();
2828
}));
2929
}));
30+
31+
client.on('error', common.expectsError({
32+
code: 'ERR_HTTP2_ERROR',
33+
name: 'Error',
34+
message: 'Protocol error'
35+
}));
3036
}));

‎test/parallel/test-http2-invalid-last-stream-id.js‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto) common.skip('missing crypto');
6+
7+
const h2 = require('http2');
8+
const net = require('net');
9+
const assert = require('assert');
10+
const { ServerHttp2Session } = require('internal/http2/core');
11+
12+
async function sendInvalidLastStreamId(server) {
13+
const client = new net.Socket();
14+
15+
const address = server.address();
16+
if (!common.hasIPv6 && address.family === 'IPv6') {
17+
// Necessary to pass CI running inside containers.
18+
client.connect(address.port);
19+
} else {
20+
client.connect(address);
21+
}
22+
23+
client.on('connect', common.mustCall(function() {
24+
// HTTP/2 preface
25+
client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8'));
26+
27+
// Empty SETTINGS frame
28+
client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]));
29+
30+
// GOAWAY frame with custom debug message
31+
const goAwayFrame = [
32+
0x00, 0x00, 0x21, // Length: 33 bytes
33+
0x07, // Type: GOAWAY
34+
0x00, // Flags
35+
0x00, 0x00, 0x00, 0x00, // Stream ID: 0
36+
0x00, 0x00, 0x00, 0x01, // Last Stream ID: 1
37+
0x00, 0x00, 0x00, 0x00, // Error Code: 0 (No error)
38+
];
39+
40+
// Add the debug message
41+
const debugMessage = 'client transport shutdown';
42+
const goAwayBuffer = Buffer.concat([
43+
Buffer.from(goAwayFrame),
44+
Buffer.from(debugMessage, 'utf8'),
45+
]);
46+
47+
client.write(goAwayBuffer);
48+
client.destroy();
49+
}));
50+
}
51+
52+
const server = h2.createServer();
53+
54+
server.on('error', common.mustNotCall());
55+
56+
server.on(
57+
'sessionError',
58+
common.mustCall((err, session) => {
59+
// When destroying the session, on Windows, we would get ECONNRESET
60+
// errors, make sure we take those into account in our tests.
61+
if (err.code !== 'ECONNRESET') {
62+
assert.strictEqual(err.code, 'ERR_HTTP2_ERROR');
63+
assert.strictEqual(err.name, 'Error');
64+
assert.strictEqual(err.message, 'Protocol error');
65+
assert.strictEqual(session instanceof ServerHttp2Session, true);
66+
}
67+
session.close();
68+
server.close();
69+
}),
70+
);
71+
72+
server.listen(
73+
0,
74+
common.mustCall(async () => {
75+
await sendInvalidLastStreamId(server);
76+
}),
77+
);

‎test/parallel/test-http2-options-max-headers-block-length.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,11 @@ server.listen(0, common.mustCall(() => {
3535
assert.strictEqual(code, h2.constants.NGHTTP2_FRAME_SIZE_ERROR);
3636
}));
3737

38+
// NGHTTP2 will automatically send the NGHTTP2_REFUSED_STREAM with
39+
// the GOAWAY frame.
3840
req.on('error', common.expectsError({
3941
code: 'ERR_HTTP2_STREAM_ERROR',
4042
name: 'Error',
41-
message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR'
43+
message: 'Stream closed with error code NGHTTP2_REFUSED_STREAM'
4244
}));
4345
}));

‎test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ server.listen(0, common.mustCall(() => {
5959
'session',
6060
common.mustCall((session) => {
6161
assert.strictEqual(session instanceof ServerHttp2Session, true);
62+
session.on('close', common.mustCall(() => {
63+
server.close();
64+
}));
6265
}),
6366
);
6467
server.on(
@@ -80,7 +83,6 @@ server.listen(0, common.mustCall(() => {
8083
assert.strictEqual(err.name, 'Error');
8184
assert.strictEqual(err.message, 'Session closed with error code 9');
8285
assert.strictEqual(session instanceof ServerHttp2Session, true);
83-
server.close();
8486
}),
8587
);
8688

‎test/parallel/test-http2-premature-close.js‎

Lines changed: 88 additions & 0 deletions

0 commit comments

Comments
 (0)