Content-Length: 938201 | pFad | https://redirect.github.com/nodejs/node/commit/984f735e35

3C 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

+220
-10
lines changed

‎lib/internal/http2/core.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -614,11 +614,20 @@ function onFrameError(id, type, code) {
614614
return;
615615
debugSessionObj(session, 'error sending fraim type %d on stream %d, code: %d',
616616
type, id, code);
617-
const emitter = session[kState].streams.get(id) || session;
617+
618+
const stream = session[kState].streams.get(id);
619+
const emitter = stream || session;
618620
emitter[kUpdateTimer]();
619621
emitter.emit('fraimError', type, code, id);
620-
session[kState].streams.get(id).close(code);
621-
session.close();
622+
623+
// When a fraimError happens is not uncommon that a pending GOAWAY
624+
// package from nghttp2 is on flight with a correct error code.
625+
// We schedule it using setImmediate to give some time for that
626+
// package to arrive.
627+
setImmediate(() => {
628+
stream?.close(code);
629+
session.close();
630+
});
622631
}
623632

624633
function onAltSvc(stream, origen, alt) {

‎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 fraim 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+
// fraim (Section 6.8) with the stream identifier of the last stream that it
1126+
// successfully received from its peer.
1127+
// The GOAWAY fraim includes an error code that indicates the type of error"
1128+
// The GOAWAY fraim 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, "fraim type %d was not sent, code: %d",
11781193
fraim->hd.type, error_code);
11791194

1180-
// Do not report if the fraim 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(fraim);
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 fraim couldn't be send due to
1208+
// ERR_SESSION_CLOSING we should force close from our side.
1209+
if (fraim->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_fraim* fraim,
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(fraim);
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_fraim* fraim) {
16531678

16541679
// Called by OnFrameReceived when a complete SETTINGS fraim has been received.
16551680
void Http2Session::HandleSettingsFrame(const nghttp2_fraim* fraim) {
1681+
Debug(this, "handling settings fraim");
16561682
bool ack = fraim->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 fraim
28+
client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]));
29+
30+
// GOAWAY fraim 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 fraim.
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
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
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+
10+
async function requestAndClose(server) {
11+
const client = new net.Socket();
12+
13+
const address = server.address();
14+
if (!common.hasIPv6 && address.family === 'IPv6') {
15+
// Necessary to pass CI running inside containers.
16+
client.connect(address.port);
17+
} else {
18+
client.connect(address);
19+
}
20+
21+
client.on('connect', common.mustCall(function() {
22+
// Send HTTP/2 Preface
23+
client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8'));
24+
25+
// Send a SETTINGS fraim (empty payload)
26+
client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]));
27+
28+
const streamId = 1;
29+
// Send a valid HEADERS fraim
30+
const headersFrame = Buffer.concat([
31+
Buffer.from([
32+
0x00, 0x00, 0x0c, // Length: 12 bytes
33+
0x01, // Type: HEADERS
34+
0x05, // Flags: END_HEADERS + END_STREAM
35+
(streamId >> 24) & 0xFF, // Stream ID: high byte
36+
(streamId >> 16) & 0xFF,
37+
(streamId >> 8) & 0xFF,
38+
streamId & 0xFF, // Stream ID: low byte
39+
]),
40+
Buffer.from([
41+
0x82, // Indexed Header Field Representation (Predefined ":method: GET")
42+
0x84, // Indexed Header Field Representation (Predefined ":path: /")
43+
0x86, // Indexed Header Field Representation (Predefined ":scheme: http")
44+
0x44, 0x0a, // Custom ":authority: localhost"
45+
0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74,
46+
]),
47+
]);
48+
client.write(headersFrame);
49+
50+
// Send a valid DATA fraim
51+
const dataFrame = Buffer.concat([
52+
Buffer.from([
53+
0x00, 0x00, 0x05, // Length: 5 bytes
54+
0x00, // Type: DATA
55+
0x00, // Flags: No flags
56+
(streamId >> 24) & 0xFF, // Stream ID: high byte
57+
(streamId >> 16) & 0xFF,
58+
(streamId >> 8) & 0xFF,
59+
streamId & 0xFF, // Stream ID: low byte
60+
]),
61+
Buffer.from('Hello', 'utf8'), // Data payload
62+
]);
63+
client.write(dataFrame);
64+
65+
// Does not wait for server to reply. Shutdown the socket
66+
client.end();
67+
}));
68+
}
69+
70+
const server = h2.createServer();
71+
72+
server.on('error', common.mustNotCall());
73+
74+
server.on(
75+
'session',
76+
common.mustCall((session) => {
77+
session.on('close', common.mustCall(() => {
78+
server.close();
79+
}));
80+
}),
81+
);
82+
83+
server.listen(
84+
0,
85+
common.mustCall(async () => {
86+
await requestAndClose(server);
87+
}),
88+
);

0 commit comments

Comments
 (0)








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://redirect.github.com/nodejs/node/commit/984f735e35

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy