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 d4787cf

Browse filesBrowse files
apapirovskiaddaleax
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. 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 2a9912c commit d4787cf
Copy full SHA for d4787cf

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
@@ -337,7 +337,7 @@ function onStreamClose(code) {
337337
`${sessionName(stream[kSession][kType])}]: closed with code ${code}`);
338338

339339
if (!stream.closed)
340-
closeStream(stream, code, false);
340+
closeStream(stream, code, kNoRstStream);
341341

342342
stream[kState].fd = -1;
343343
// Defer destroy we actually emit end.
@@ -1476,7 +1476,11 @@ function finishSendTrailers(stream, headersList) {
14761476
stream[kMaybeDestroy]();
14771477
}
14781478

1479-
function closeStream(stream, code, shouldSubmitRstStream = true) {
1479+
const kNoRstStream = 0;
1480+
const kSubmitRstStream = 1;
1481+
const kForceRstStream = 2;
1482+
1483+
function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
14801484
const state = stream[kState];
14811485
state.flags |= STREAM_FLAGS_CLOSED;
14821486
state.rstCode = code;
@@ -1499,9 +1503,10 @@ function closeStream(stream, code, shouldSubmitRstStream = true) {
14991503
stream.end();
15001504
}
15011505

1502-
if (shouldSubmitRstStream) {
1506+
if (rstStreamStatus !== kNoRstStream) {
15031507
const finishFn = finishCloseStream.bind(stream, code);
1504-
if (!ending || finished || code !== NGHTTP2_NO_ERROR)
1508+
if (!ending || finished || code !== NGHTTP2_NO_ERROR ||
1509+
rstStreamStatus === kForceRstStream)
15051510
finishFn();
15061511
else
15071512
stream.once('finish', finishFn);
@@ -1852,7 +1857,7 @@ class Http2Stream extends Duplex {
18521857
const hasHandle = handle !== undefined;
18531858

18541859
if (!this.closed)
1855-
closeStream(this, code, hasHandle);
1860+
closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream);
18561861
this.push(null);
18571862

18581863
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
@@ -1762,6 +1762,8 @@ void Http2Stream::Destroy() {
17621762
// Do nothing if this stream instance is already destroyed
17631763
if (IsDestroyed())
17641764
return;
1765+
if (session_->HasPendingRstStream(id_))
1766+
FlushRstStream();
17651767
flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED;
17661768

17671769
Debug(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 {
@@ -803,6 +804,12 @@ class Http2Session : public AsyncWrap, public StreamListener {
803804
pending_rst_streams_.emplace_back(stream_id);
804805
}
805806

807+
inline bool HasPendingRstStream(int32_t stream_id) {
808+
return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(),
809+
pending_rst_streams_.end(),
810+
stream_id);
811+
}
812+
806813
// Handle reads/writes from the underlying network transport.
807814
void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
808815
void OnStreamAfterWrite(WriteWrap* w, int status) override;
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.