Skip to content

Commit 9a6ea7e

Browse files
jasnellMylesBorins
jasnell
authored and
MylesBorins
committed
http2: properly handle already closed stream error
Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17942 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 0078a97 commit 9a6ea7e

File tree

4 files changed

+220
-0
lines changed

4 files changed

+220
-0
lines changed

‎src/node_http2.cc

+27
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,8 @@ Http2Session::Callbacks::Callbacks(bool kHasGetPaddingCallback) {
457457
callbacks, OnNghttpError);
458458
nghttp2_session_callbacks_set_send_data_callback(
459459
callbacks, OnSendData);
460+
nghttp2_session_callbacks_set_on_invalid_frame_recv_callback(
461+
callbacks, OnInvalidFrame);
460462

461463
if (kHasGetPaddingCallback) {
462464
nghttp2_session_callbacks_set_select_padding_callback(
@@ -881,6 +883,31 @@ inline int Http2Session::OnFrameReceive(nghttp2_session* handle,
881883
return 0;
882884
}
883885

886+
inline int Http2Session::OnInvalidFrame(nghttp2_session* handle,
887+
const nghttp2_frame *frame,
888+
int lib_error_code,
889+
void* user_data) {
890+
Http2Session* session = static_cast<Http2Session*>(user_data);
891+
892+
DEBUG_HTTP2SESSION2(session, "invalid frame received, code: %d",
893+
lib_error_code);
894+
895+
// If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
896+
if (nghttp2_is_fatal(lib_error_code) ||
897+
lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) {
898+
Environment* env = session->env();
899+
Isolate* isolate = env->isolate();
900+
HandleScope scope(isolate);
901+
Local<Context> context = env->context();
902+
Context::Scope context_scope(context);
903+
904+
Local<Value> argv[1] = {
905+
Integer::New(isolate, lib_error_code),
906+
};
907+
session->MakeCallback(env->error_string(), arraysize(argv), argv);
908+
}
909+
return 0;
910+
}
884911

885912
// If nghttp2 is unable to send a queued up frame, it will call this callback
886913
// to let us know. If the failure occurred because we are in the process of

‎src/node_http2.h

+5
Original file line numberDiff line numberDiff line change
@@ -992,6 +992,11 @@ class Http2Session : public AsyncWrap {
992992
size_t length,
993993
nghttp2_data_source* source,
994994
void* user_data);
995+
static inline int OnInvalidFrame(
996+
nghttp2_session* session,
997+
const nghttp2_frame *frame,
998+
int lib_error_code,
999+
void* user_data);
9951000

9961001

9971002
static inline ssize_t OnStreamReadFD(

‎test/common/http2.js

+129
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/* eslint-disable required-modules */
2+
'use strict';
3+
4+
// An HTTP/2 testing tool used to create mock frames for direct testing
5+
// of HTTP/2 endpoints.
6+
7+
const kFrameData = Symbol('frame-data');
8+
const FLAG_EOS = 0x1;
9+
const FLAG_ACK = 0x1;
10+
const FLAG_EOH = 0x4;
11+
const FLAG_PADDED = 0x8;
12+
const PADDING = Buffer.alloc(255);
13+
14+
const kClientMagic = Buffer.from('505249202a20485454502f322' +
15+
'e300d0a0d0a534d0d0a0d0a', 'hex');
16+
17+
const kFakeRequestHeaders = Buffer.from('828684410f7777772e65' +
18+
'78616d706c652e636f6d', 'hex');
19+
20+
21+
const kFakeResponseHeaders = Buffer.from('4803333032580770726976617465611d' +
22+
'4d6f6e2c203231204f63742032303133' +
23+
'2032303a31333a323120474d546e1768' +
24+
'747470733a2f2f7777772e6578616d70' +
25+
'6c652e636f6d', 'hex');
26+
27+
function isUint32(val) {
28+
return val >>> 0 === val;
29+
}
30+
31+
function isUint24(val) {
32+
return val >>> 0 === val && val <= 0xFFFFFF;
33+
}
34+
35+
function isUint8(val) {
36+
return val >>> 0 === val && val <= 0xFF;
37+
}
38+
39+
function write32BE(array, pos, val) {
40+
if (!isUint32(val))
41+
throw new RangeError('val is not a 32-bit number');
42+
array[pos++] = (val >> 24) & 0xff;
43+
array[pos++] = (val >> 16) & 0xff;
44+
array[pos++] = (val >> 8) & 0xff;
45+
array[pos++] = val & 0xff;
46+
}
47+
48+
function write24BE(array, pos, val) {
49+
if (!isUint24(val))
50+
throw new RangeError('val is not a 24-bit number');
51+
array[pos++] = (val >> 16) & 0xff;
52+
array[pos++] = (val >> 8) & 0xff;
53+
array[pos++] = val & 0xff;
54+
}
55+
56+
function write8(array, pos, val) {
57+
if (!isUint8(val))
58+
throw new RangeError('val is not an 8-bit number');
59+
array[pos] = val;
60+
}
61+
62+
class Frame {
63+
constructor(length, type, flags, id) {
64+
this[kFrameData] = Buffer.alloc(9);
65+
write24BE(this[kFrameData], 0, length);
66+
write8(this[kFrameData], 3, type);
67+
write8(this[kFrameData], 4, flags);
68+
write32BE(this[kFrameData], 5, id);
69+
}
70+
71+
get data() {
72+
return this[kFrameData];
73+
}
74+
}
75+
76+
class SettingsFrame extends Frame {
77+
constructor(ack = false) {
78+
let flags = 0;
79+
if (ack)
80+
flags |= FLAG_ACK;
81+
super(0, 4, flags, 0);
82+
}
83+
}
84+
85+
class DataFrame extends Frame {
86+
constructor(id, payload, padlen = 0, final = false) {
87+
let len = payload.length;
88+
let flags = 0;
89+
if (final) flags |= FLAG_EOS;
90+
const buffers = [payload];
91+
if (padlen > 0) {
92+
buffers.unshift(Buffer.from([padlen]));
93+
buffers.push(PADDING.slice(0, padlen));
94+
len += padlen + 1;
95+
flags |= FLAG_PADDED;
96+
}
97+
super(len, 0, flags, id);
98+
buffers.unshift(this[kFrameData]);
99+
this[kFrameData] = Buffer.concat(buffers);
100+
}
101+
}
102+
103+
class HeadersFrame extends Frame {
104+
constructor(id, payload, padlen = 0, final = false) {
105+
let len = payload.length;
106+
let flags = FLAG_EOH;
107+
if (final) flags |= FLAG_EOS;
108+
const buffers = [payload];
109+
if (padlen > 0) {
110+
buffers.unshift(Buffer.from([padlen]));
111+
buffers.push(PADDING.slice(0, padlen));
112+
len += padlen + 1;
113+
flags |= FLAG_PADDED;
114+
}
115+
super(len, 1, flags, id);
116+
buffers.unshift(this[kFrameData]);
117+
this[kFrameData] = Buffer.concat(buffers);
118+
}
119+
}
120+
121+
module.exports = {
122+
Frame,
123+
DataFrame,
124+
HeadersFrame,
125+
SettingsFrame,
126+
kFakeRequestHeaders,
127+
kFakeResponseHeaders,
128+
kClientMagic
129+
};

‎test/parallel/test-http2-misbehaving-multiplex.js

+59
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const h2 = require('http2');
9+
const net = require('net');
10+
const h2test = require('../common/http2');
11+
let client;
12+
13+
const server = h2.createServer();
14+
server.on('stream', common.mustCall((stream) => {
15+
stream.respond();
16+
stream.end('ok');
17+
}, 2));
18+
server.on('session', common.mustCall((session) => {
19+
session.on('error', common.expectsError({
20+
code: 'ERR_HTTP2_ERROR',
21+
type: Error,
22+
message: 'Stream was already closed or invalid'
23+
}));
24+
}));
25+
26+
const settings = new h2test.SettingsFrame();
27+
const settingsAck = new h2test.SettingsFrame(true);
28+
const head1 = new h2test.HeadersFrame(1, h2test.kFakeRequestHeaders, 0, true);
29+
const head2 = new h2test.HeadersFrame(3, h2test.kFakeRequestHeaders, 0, true);
30+
const head3 = new h2test.HeadersFrame(1, h2test.kFakeRequestHeaders, 0, true);
31+
const head4 = new h2test.HeadersFrame(5, h2test.kFakeRequestHeaders, 0, true);
32+
33+
server.listen(0, () => {
34+
client = net.connect(server.address().port, () => {
35+
client.write(h2test.kClientMagic, () => {
36+
client.write(settings.data, () => {
37+
client.write(settingsAck.data);
38+
// This will make it ok.
39+
client.write(head1.data, () => {
40+
// This will make it ok.
41+
client.write(head2.data, () => {
42+
// This will cause an error to occur because the client is
43+
// attempting to reuse an already closed stream. This must
44+
// cause the server session to be torn down.
45+
client.write(head3.data, () => {
46+
// This won't ever make it to the server
47+
client.write(head4.data);
48+
});
49+
});
50+
});
51+
});
52+
});
53+
});
54+
55+
// An error may or may not be emitted on the client side, we don't care
56+
// either way if it is, but we don't want to die if it is.
57+
client.on('error', () => {});
58+
client.on('close', common.mustCall(() => server.close()));
59+
});

0 commit comments

Comments
 (0)