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 b22266c

Browse filesBrowse files
apapirovskiBethGriggs
authored andcommitted
http2: force through RST_STREAM in destroy
If still needed, force through RST_STREAM in Http2Stream#destroy calls, so that nghttp2 can wrap up properly and doesn't continue trying to read & write data to the stream. Backport-PR-URL: #22850 PR-URL: #21016 Fixes: #21008 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent 91be1dc commit b22266c
Copy full SHA for b22266c

File tree

Expand file treeCollapse file tree

4 files changed

+59
-5
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+59
-5
lines changed
Open diff view settings
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+10-5Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ function onStreamClose(code) {
286286
`${sessionName(stream[kSession][kType])}]: closed with code ${code}`);
287287

288288
if (!stream.closed)
289-
closeStream(stream, code, false);
289+
closeStream(stream, code, kNoRstStream);
290290

291291
if (stream[kState].fd !== undefined)
292292
tryClose(stream[kState].fd);
@@ -1458,7 +1458,11 @@ function finishSendTrailers(stream, headersList) {
14581458
stream[kMaybeDestroy]();
14591459
}
14601460

1461-
function closeStream(stream, code, shouldSubmitRstStream = true) {
1461+
const kNoRstStream = 0;
1462+
const kSubmitRstStream = 1;
1463+
const kForceRstStream = 2;
1464+
1465+
function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
14621466
const state = stream[kState];
14631467
state.flags |= STREAM_FLAGS_CLOSED;
14641468
state.rstCode = code;
@@ -1481,9 +1485,10 @@ function closeStream(stream, code, shouldSubmitRstStream = true) {
14811485
stream.end();
14821486
}
14831487

1484-
if (shouldSubmitRstStream) {
1488+
if (rstStreamStatus !== kNoRstStream) {
14851489
const finishFn = finishCloseStream.bind(stream, code);
1486-
if (!ending || finished || code !== NGHTTP2_NO_ERROR)
1490+
if (!ending || finished || code !== NGHTTP2_NO_ERROR ||
1491+
rstStreamStatus === kForceRstStream)
14871492
finishFn();
14881493
else
14891494
stream.once('finish', finishFn);
@@ -1845,7 +1850,7 @@ class Http2Stream extends Duplex {
18451850
const hasHandle = handle !== undefined;
18461851

18471852
if (!this.closed)
1848-
closeStream(this, code, hasHandle);
1853+
closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream);
18491854
this.push(null);
18501855

18511856
if (hasHandle) {
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1839,6 +1839,8 @@ inline void Http2Stream::Destroy() {
18391839
// Do nothing if this stream instance is already destroyed
18401840
if (IsDestroyed())
18411841
return;
1842+
if (session_->HasPendingRstStream(id_))
1843+
FlushRstStream();
18421844
flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED;
18431845

18441846
DEBUG_HTTP2STREAM(this, "destroying stream");
Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "stream_base-inl.h"
1010
#include "string_bytes.h"
1111

12+
#include <algorithm>
1213
#include <queue>
1314

1415
namespace node {
@@ -880,6 +881,12 @@ class Http2Session : public AsyncWrap {
880881
pending_rst_streams_.emplace_back(stream_id);
881882
}
882883

884+
inline bool HasPendingRstStream(int32_t stream_id) {
885+
return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(),
886+
pending_rst_streams_.end(),
887+
stream_id);
888+
}
889+
883890
static void OnStreamAllocImpl(size_t suggested_size,
884891
uv_buf_t* buf,
885892
void* ctx);
Collapse file
+40Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
const fixtures = require('../common/fixtures');
6+
const http2 = require('http2');
7+
8+
// This test will result in a crash due to a missed CHECK in C++ or
9+
// a straight-up segfault if the C++ doesn't send RST_STREAM through
10+
// properly when calling destroy.
11+
12+
const content = Buffer.alloc(60000, 0x44);
13+
14+
const server = http2.createSecureServer({
15+
key: fixtures.readKey('agent1-key.pem'),
16+
cert: fixtures.readKey('agent1-cert.pem')
17+
});
18+
server.on('stream', common.mustCall((stream) => {
19+
stream.respond({
20+
'Content-Type': 'application/octet-stream',
21+
'Content-Length': (content.length.toString() * 2),
22+
'Vary': 'Accept-Encoding'
23+
}, { waitForTrailers: true });
24+
25+
stream.write(content);
26+
stream.destroy();
27+
}));
28+
29+
server.listen(0, common.mustCall(() => {
30+
const client = http2.connect(`https://localhost:${server.address().port}`,
31+
{ rejectUnauthorized: false });
32+
33+
const req = client.request({ ':path': '/' });
34+
req.end();
35+
36+
req.on('close', common.mustCall(() => {
37+
client.close();
38+
server.close();
39+
}));
40+
}));

0 commit comments

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