Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d29fbc0

Browse filesBrowse files
pimterryaduh95
pimterry
authored and
aduh95
committed
deps: fix integration issues with the latest nghttp2
This is a set of src & tests fixes for nghttp2 due to changes in v1.67.0+ which require a selection of changes to how we handle low-level protocol errors when using the latest versions of nghttp2, changing both some src error handling and updating some tests to match. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62891 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 537825a commit d29fbc0
Copy full SHA for d29fbc0

7 files changed

+177-30Lines changed: 177 additions & 30 deletions
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+31Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,6 +1265,24 @@ int Http2Session::OnFrameSent(nghttp2_session* handle,
12651265
void* user_data) {
12661266
Http2Session* session = static_cast<Http2Session*>(user_data);
12671267
session->statistics_.frame_sent += 1;
1268+
1269+
// If nghttp2 has internally terminated the session (e.g. due to a protocol
1270+
// error like oversized frames, padding errors, or HPACK compression
1271+
// failures), it calls nghttp2_session_terminate_session() directly which
1272+
// queues a GOAWAY but does not invoke any application-level callback.
1273+
// Detect that case here: a GOAWAY was sent but we never initiated it
1274+
// (no Close(), no session.close(), no session.goaway()).
1275+
//
1276+
// We set a flag here, and then throw the error at the end of
1277+
// SendPendingData, to wait until the GOAWAY is written before the session
1278+
// is torn down.
1279+
if (frame->hd.type == NGHTTP2_GOAWAY && !session->is_closing() &&
1280+
!session->is_destroyed() && !session->IsGracefulCloseInitiated() &&
1281+
!session->goaway_initiated_) {
1282+
Debug(session, "nghttp2 session terminated internally");
1283+
session->internal_goaway_sent_ = true;
1284+
}
1285+
12681286
return 0;
12691287
}
12701288

@@ -1999,6 +2017,18 @@ uint8_t Http2Session::SendPendingData() {
19992017

20002018
MaybeStopReading();
20012019

2020+
// If nghttp2 has internally torn down the session (detected in OnFrameSent)
2021+
// during the nghttp2_session_mem_send loop above, at this point we error:
2022+
if (internal_goaway_sent_) {
2023+
internal_goaway_sent_ = false;
2024+
if (!is_closing() && !is_destroyed()) {
2025+
Isolate* isolate = env()->isolate();
2026+
HandleScope scope(isolate);
2027+
Local<Value> arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
2028+
MakeCallback(env()->http2session_on_error_function(), 1, &arg);
2029+
}
2030+
}
2031+
20022032
return 0;
20032033
}
20042034

@@ -2940,6 +2970,7 @@ void Http2Session::Goaway(uint32_t code,
29402970
if (is_destroyed())
29412971
return;
29422972

2973+
goaway_initiated_ = true;
29432974
Http2Scope h2scope(this);
29442975
// the last proc stream id is the most recently created Http2Stream.
29452976
if (lastStreamID <= 0)
Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -971,6 +971,8 @@ class Http2Session : public AsyncWrap,
971971

972972
// Flag to indicate that JavaScript has initiated a graceful closure
973973
bool graceful_close_initiated_ = false;
974+
bool goaway_initiated_ = false;
975+
bool internal_goaway_sent_ = false;
974976
};
975977

976978
struct Http2SessionPerformanceEntryTraits {
Collapse file

‎test/parallel/test-http2-misbehaving-flow-control-paused.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http2-misbehaving-flow-control-paused.js
+19-10Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,23 +47,29 @@ const data = Buffer.from([
4747
// Bad client! Bad!
4848
//
4949
// Fortunately, nghttp2 handles this situation for us by keeping track
50-
// of the flow control window and responding with a FLOW_CONTROL_ERROR
51-
// causing the stream to get shut down...
52-
//
53-
// At least, that's what is supposed to happen.
50+
// of the flow control window and sending GOAWAY to end the session.
5451

5552
let client;
5653

5754
const server = h2.createServer({ settings: { initialWindowSize: 36 } });
55+
56+
server.on('session', common.mustCall((session) => {
57+
session.on('error', common.expectsError({
58+
code: 'ERR_HTTP2_ERROR',
59+
name: 'Error',
60+
message: 'Protocol error'
61+
}));
62+
}));
63+
5864
server.on('stream', common.mustCall((stream) => {
5965
// Set the high water mark to zero, since otherwise we still accept
6066
// reads from the source stream (if we can consume them).
6167
stream._readableState.highWaterMark = 0;
6268
stream.pause();
6369
stream.on('error', common.expectsError({
64-
code: 'ERR_HTTP2_STREAM_ERROR',
70+
code: 'ERR_HTTP2_ERROR',
6571
name: 'Error',
66-
message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
72+
message: 'Protocol error'
6773
}));
6874
stream.on('close', common.mustCall(() => {
6975
server.close();
@@ -72,11 +78,14 @@ server.on('stream', common.mustCall((stream) => {
7278
stream.on('end', common.mustNotCall());
7379
}));
7480

75-
server.listen(0, () => {
76-
client = net.connect(server.address().port, () => {
81+
server.listen(0, common.mustCall(() => {
82+
client = net.connect(server.address().port, common.mustCall(() => {
7783
client.write(preamble);
7884
client.write(data);
7985
client.write(data);
8086
client.write(data);
81-
});
82-
});
87+
88+
// TCP connection is closed by the server
89+
client.on('close', common.mustCall());
90+
}));
91+
}));
Collapse file

‎test/parallel/test-http2-misbehaving-flow-control.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http2-misbehaving-flow-control.js
+19-10Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,24 @@ const data = Buffer.from([
5858
// Bad client! Bad!
5959
//
6060
// Fortunately, nghttp2 handles this situation for us by keeping track
61-
// of the flow control window and responding with a FLOW_CONTROL_ERROR
62-
// causing the stream to get shut down...
63-
//
64-
// At least, that's what is supposed to happen.
61+
// of the flow control window and sending GOAWAY to end the session.
6562

6663
let client;
6764
const server = h2.createServer({ settings: { initialWindowSize: 18 } });
65+
66+
server.on('session', common.mustCall((session) => {
67+
session.on('error', common.expectsError({
68+
code: 'ERR_HTTP2_ERROR',
69+
name: 'Error',
70+
message: 'Protocol error'
71+
}));
72+
}));
73+
6874
server.on('stream', common.mustCall((stream) => {
6975
stream.on('error', common.expectsError({
70-
code: 'ERR_HTTP2_STREAM_ERROR',
76+
code: 'ERR_HTTP2_ERROR',
7177
name: 'Error',
72-
message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
78+
message: 'Protocol error'
7379
}));
7480
stream.on('close', common.mustCall(() => {
7581
server.close(common.mustCall());
@@ -82,10 +88,13 @@ server.on('stream', common.mustCall((stream) => {
8288

8389
server.on('close', common.mustCall());
8490

85-
server.listen(0, () => {
86-
client = net.connect(server.address().port, () => {
91+
server.listen(0, common.mustCall(() => {
92+
client = net.connect(server.address().port, common.mustCall(() => {
8793
client.write(preamble);
8894
client.write(data);
8995
client.write(data);
90-
});
91-
});
96+
97+
// TCP connection is closed by the server
98+
client.on('close', common.mustCall());
99+
}));
100+
}));
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
+10-2Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,19 @@ server.listen(0, common.mustCall(() => {
9090
0,
9191
common.mustCall(() => {
9292
const client = h2.connect(`http://localhost:${server.address().port}`);
93-
client.on('error', common.mustNotCall());
93+
// The server sends oversized headers that cause a compression error on
94+
// the client side, so nghttp2 internally terminates the client session.
95+
client.on('error', common.expectsError({
96+
code: 'ERR_HTTP2_ERROR',
97+
name: 'Error',
98+
}));
9499

95100
const req = client.request();
96101
req.on('response', common.mustNotCall());
97-
req.on('error', common.mustNotCall());
102+
req.on('error', common.expectsError({
103+
code: 'ERR_HTTP2_ERROR',
104+
name: 'Error',
105+
}));
98106
req.end();
99107
}),
100108
);
Collapse file

‎test/parallel/test-http2-session-cleanup-on-nghttp2-goaway.js‎

Copy file name to clipboard
+91Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const http2 = require('http2');
9+
const net = require('net');
10+
11+
// When nghttp2 internally sends a GOAWAY frame due to a protocol error, it
12+
// may call nghttp2_session_terminate_session() directly, bypassing the
13+
// on_invalid_frame_recv_callback entirely. This test ensures that even
14+
// in that scenario, we still correctly clean up the session & connection.
15+
//
16+
// This test reproduces this with a client who sends a frame header with
17+
// a length exceeding the default max_frame_size (16384). nghttp2 responds
18+
// with GOAWAY(FRAME_SIZE_ERROR) without notifying Node through any callback.
19+
20+
const server = http2.createServer();
21+
22+
server.on('session', common.mustCall((session) => {
23+
session.on('error', common.expectsError({
24+
code: 'ERR_HTTP2_ERROR',
25+
name: 'Error',
26+
message: 'Protocol error'
27+
}));
28+
29+
session.on('close', common.mustCall(() => server.close()));
30+
}));
31+
32+
server.listen(0, common.mustCall(() => {
33+
const conn = net.connect({
34+
port: server.address().port,
35+
allowHalfOpen: true,
36+
});
37+
38+
// HTTP/2 client connection preface.
39+
conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n');
40+
41+
// Empty SETTINGS frame.
42+
const settingsFrame = Buffer.alloc(9);
43+
settingsFrame[3] = 0x04; // type: SETTINGS
44+
conn.write(settingsFrame);
45+
46+
let inbuf = Buffer.alloc(0);
47+
let state = 'settingsHeader';
48+
let settingsFrameLength;
49+
50+
conn.on('data', (chunk) => {
51+
inbuf = Buffer.concat([inbuf, chunk]);
52+
53+
switch (state) {
54+
case 'settingsHeader':
55+
if (inbuf.length < 9) return;
56+
settingsFrameLength = inbuf.readUIntBE(0, 3);
57+
inbuf = inbuf.slice(9);
58+
state = 'readingSettings';
59+
// Fallthrough
60+
case 'readingSettings': {
61+
if (inbuf.length < settingsFrameLength) return;
62+
inbuf = inbuf.slice(settingsFrameLength);
63+
state = 'done';
64+
65+
// ACK the server SETTINGS.
66+
const ack = Buffer.alloc(9);
67+
ack[3] = 0x04; // type: SETTINGS
68+
ack[4] = 0x01; // flag: ACK
69+
conn.write(ack);
70+
71+
// Send a HEADERS frame header claiming length 16385, which exceeds
72+
// the default max_frame_size of 16384. nghttp2 checks the length
73+
// before reading any payload, so no body is needed. This triggers
74+
// nghttp2_session_terminate_session(FRAME_SIZE_ERROR) directly in
75+
// nghttp2_session_mem_recv2 — bypassing on_invalid_frame_recv_callback.
76+
const oversized = Buffer.alloc(9);
77+
oversized.writeUIntBE(16385, 0, 3); // length: 16385 (one over max)
78+
oversized[3] = 0x01; // type: HEADERS
79+
oversized[4] = 0x04; // flags: END_HEADERS
80+
oversized.writeUInt32BE(1, 5); // stream id: 1
81+
conn.write(oversized);
82+
83+
// No need to write the data - the header alone triggers the check.
84+
}
85+
}
86+
});
87+
88+
// The server must close the connection after sending GOAWAY:
89+
conn.on('end', common.mustCall(() => conn.end()));
90+
conn.on('close', common.mustCall());
91+
}));
Collapse file

‎tools/nix/sharedLibDeps.nix‎

Copy file name to clipboardExpand all lines: tools/nix/sharedLibDeps.nix
+5-8Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,11 @@
2525
hdr-histogram = pkgs.hdrhistogram_c;
2626
http-parser = pkgs.llhttp;
2727
nghttp2 = pkgs.nghttp2.overrideAttrs {
28-
patches = [
29-
(pkgs.fetchpatch2 {
30-
url = "https://github.com/nghttp2/nghttp2/commit/7784fa979d0bcf801a35f1afbb25fb048d815cd7.patch?full_index=1";
31-
revert = true;
32-
excludes = [ "lib/includes/nghttp2/nghttp2.h" ];
33-
hash = "sha256-RG87Qifjpl7HTP9ac2JwHj2XAbDlFgOpAnpZX3ET6gU=";
34-
})
35-
];
28+
version = "1.69.0";
29+
src = pkgs.fetchurl {
30+
url = "https://github.com/nghttp2/nghttp2/releases/download/v1.69.0/nghttp2-1.69.0.tar.bz2";
31+
hash = "sha256-PxhfWxw+d4heuc8/LE2ksan3OiS/WVe4KRg60Tf4Lcg=";
32+
};
3633
};
3734
}
3835
// (pkgs.lib.optionalAttrs withLief {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.