Skip to content

Commit 41b1e68

Browse files
authored
http2: detect streams abandoned by nghttp2
When a peer sends GOAWAY(NO_ERROR), the session starts a graceful close on its own, which disarmed the detection of nghttp2 internally terminating the session after a protocol error. Instead, also treat it as an internal termination when nghttp2 no longer wants to read or write while we still have open streams. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #66641 Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent 6dea43a commit 41b1e68

3 files changed

Lines changed: 134 additions & 3 deletions

File tree

‎src/node_http2.cc‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -801,6 +801,18 @@ bool Http2Session::HasPendingData() const {
801801
return true;
802802
}
803803

804+
// nghttp2 signals that the connection should be dropped by having both
805+
// want_read and want_write return 0. If that happens while we still track
806+
// open streams, nghttp2 has terminated the session on its own (for example
807+
// due to a protocol error from the peer) and those streams will never be
808+
// completed.
809+
bool Http2Session::HasStreamsAbandonedByNghttp2() const {
810+
if (HasPendingData()) return false;
811+
return std::ranges::any_of(streams_, [](const auto& entry) {
812+
return !entry.second->is_closed() && !entry.second->is_destroyed();
813+
});
814+
}
815+
804816
void Http2Session::EmitStatistics() {
805817
if (!HasHttp2Observer(env())) [[likely]] {
806818
return;
@@ -2069,9 +2081,11 @@ uint8_t Http2Session::SendPendingData() {
20692081

20702082
MaybeStopReading();
20712083

2072-
// If nghttp2 has internally torn down the session (detected in OnFrameSent)
2073-
// during the nghttp2_session_mem_send loop above, at this point we error:
2074-
if (internal_goaway_sent_) {
2084+
// If nghttp2 has internally torn down the session (detected in OnFrameSent,
2085+
// or because it no longer wants to read or write while we still have open
2086+
// streams) during the nghttp2_session_mem_send loop above, at this point we
2087+
// error:
2088+
if (internal_goaway_sent_ || HasStreamsAbandonedByNghttp2()) {
20752089
internal_goaway_sent_ = false;
20762090
if (!is_closing() && !is_destroyed()) {
20772091
Isolate* isolate = env()->isolate();

‎src/node_http2.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -794,6 +794,7 @@ class Http2Session : public AsyncWrap,
794794
BaseObjectPtr<Http2Ping> PopPing();
795795
bool AddPing(const uint8_t* data, v8::Local<v8::Function> callback);
796796
bool HasPendingData() const;
797+
bool HasStreamsAbandonedByNghttp2() const;
797798

798799
BaseObjectPtr<Http2Settings> PopSettings();
799800
bool AddSettings(v8::Local<v8::Function> callback);
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
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 a peer sends GOAWAY(NO_ERROR), the session starts a graceful close
12+
// on its own. If nghttp2 later terminates the session internally because of
13+
// a protocol error (here, an oversized frame) while a stream is still open,
14+
// the session must still be torn down and the error surfaced, rather than
15+
// leaving the stream and connection hanging.
16+
17+
const server = http2.createServer();
18+
19+
server.on('stream', common.mustCall((stream) => {
20+
stream.on('error', common.expectsError({
21+
code: 'ERR_HTTP2_ERROR',
22+
name: 'Error',
23+
message: 'Protocol error'
24+
}));
25+
stream.on('close', common.mustCall());
26+
}));
27+
28+
server.on('session', common.mustCall((session) => {
29+
session.on('goaway', common.mustCall());
30+
session.on('error', common.expectsError({
31+
code: 'ERR_HTTP2_ERROR',
32+
name: 'Error',
33+
message: 'Protocol error'
34+
}));
35+
36+
session.on('close', common.mustCall(() => server.close()));
37+
}));
38+
39+
server.listen(0, common.mustCall(() => {
40+
const conn = net.connect({
41+
port: server.address().port,
42+
allowHalfOpen: true,
43+
});
44+
45+
// HTTP/2 client connection preface.
46+
conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n');
47+
48+
// Empty SETTINGS frame.
49+
const settingsFrame = Buffer.alloc(9);
50+
settingsFrame[3] = 0x04; // type: SETTINGS
51+
conn.write(settingsFrame);
52+
53+
let inbuf = Buffer.alloc(0);
54+
let state = 'settingsHeader';
55+
let settingsFrameLength;
56+
57+
conn.on('data', (chunk) => {
58+
inbuf = Buffer.concat([inbuf, chunk]);
59+
60+
switch (state) {
61+
case 'settingsHeader':
62+
if (inbuf.length < 9) return;
63+
settingsFrameLength = inbuf.readUIntBE(0, 3);
64+
inbuf = inbuf.slice(9);
65+
state = 'readingSettings';
66+
// Fallthrough
67+
case 'readingSettings': {
68+
if (inbuf.length < settingsFrameLength) return;
69+
inbuf = inbuf.slice(settingsFrameLength);
70+
state = 'done';
71+
72+
// ACK the server SETTINGS.
73+
const ack = Buffer.alloc(9);
74+
ack[3] = 0x04; // type: SETTINGS
75+
ack[4] = 0x01; // flag: ACK
76+
conn.write(ack);
77+
78+
// Open stream 1 without END_STREAM, so it stays open on the server.
79+
// HPACK: :method GET (0x82), :scheme http (0x86), :path / (0x84),
80+
// :authority localhost (literal with indexed name 1).
81+
const block = Buffer.from([
82+
0x82, 0x86, 0x84, 0x41, 0x09,
83+
...Buffer.from('localhost'),
84+
]);
85+
const headers = Buffer.alloc(9);
86+
headers.writeUIntBE(block.length, 0, 3);
87+
headers[3] = 0x01; // type: HEADERS
88+
headers[4] = 0x04; // flags: END_HEADERS
89+
headers.writeUInt32BE(1, 5); // stream id: 1
90+
conn.write(Buffer.concat([headers, block]));
91+
92+
// GOAWAY(NO_ERROR), last stream id 0. This makes the server
93+
// gracefully close its session without any application code.
94+
const goaway = Buffer.alloc(17);
95+
goaway.writeUIntBE(8, 0, 3); // length: 8
96+
goaway[3] = 0x07; // type: GOAWAY
97+
goaway.writeUInt32BE(0, 9); // last stream id
98+
goaway.writeUInt32BE(0, 13); // error code: NO_ERROR
99+
conn.write(goaway);
100+
101+
// Send a DATA frame header claiming length 16385, which exceeds the
102+
// default max_frame_size of 16384, so nghttp2 calls
103+
// nghttp2_session_terminate_session(FRAME_SIZE_ERROR) internally.
104+
const oversized = Buffer.alloc(9);
105+
oversized.writeUIntBE(16385, 0, 3); // length: 16385 (one over max)
106+
oversized[3] = 0x00; // type: DATA
107+
oversized.writeUInt32BE(1, 5); // stream id: 1
108+
conn.write(oversized);
109+
}
110+
}
111+
});
112+
113+
// The server must close the connection after sending GOAWAY:
114+
conn.on('end', common.mustCall(() => conn.end()));
115+
conn.on('close', common.mustCall());
116+
}));

0 commit comments

Comments
 (0)