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 50d1233

Browse filesBrowse files
addaleaxMylesBorins
authored andcommitted
http2: no stream destroy while its data is on the wire
This fixes a crash that occurred when a `Http2Stream` write is completed after it is already destroyed. Instead, don’t destroy the stream in that case and wait for GC to take over. Backport-PR-URL: #19185 PR-URL: #19002 Fixes: #18973 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
1 parent 420d56c commit 50d1233
Copy full SHA for 50d1233

File tree

Expand file treeCollapse file tree

3 files changed

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

3 files changed

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

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+15-5Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1700,6 +1700,14 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
17001700
stream_buf_ = uv_buf_init(nullptr, 0);
17011701
}
17021702

1703+
bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) {
1704+
for (const nghttp2_stream_write& wr : outgoing_buffers_) {
1705+
if (wr.req_wrap != nullptr && wr.req_wrap->stream() == stream)
1706+
return true;
1707+
}
1708+
return false;
1709+
}
1710+
17031711
// Every Http2Session session is tightly bound to a single i/o StreamBase
17041712
// (typically a net.Socket or tls.TLSSocket). The lifecycle of the two is
17051713
// tightly coupled with all data transfer between the two happening at the
@@ -1753,13 +1761,11 @@ Http2Stream::Http2Stream(
17531761

17541762

17551763
Http2Stream::~Http2Stream() {
1764+
DEBUG_HTTP2STREAM(this, "tearing down stream");
17561765
if (session_ != nullptr) {
17571766
session_->RemoveStream(this);
17581767
session_ = nullptr;
17591768
}
1760-
1761-
if (!object().IsEmpty())
1762-
ClearWrap(object());
17631769
}
17641770

17651771
// Notify the Http2Stream that a new block of HEADERS is being processed.
@@ -1837,15 +1843,19 @@ inline void Http2Stream::Destroy() {
18371843
Http2Stream* stream = static_cast<Http2Stream*>(data);
18381844
// Free any remaining outgoing data chunks here. This should be done
18391845
// here because it's possible for destroy to have been called while
1840-
// we still have qeueued outbound writes.
1846+
// we still have queued outbound writes.
18411847
while (!stream->queue_.empty()) {
18421848
nghttp2_stream_write& head = stream->queue_.front();
18431849
if (head.req_wrap != nullptr)
18441850
head.req_wrap->Done(UV_ECANCELED);
18451851
stream->queue_.pop();
18461852
}
18471853

1848-
delete stream;
1854+
// We can destroy the stream now if there are no writes for it
1855+
// already on the socket. Otherwise, we'll wait for the garbage collector
1856+
// to take care of cleaning up.
1857+
if (!stream->session()->HasWritesOnSocketForStream(stream))
1858+
delete stream;
18491859
}, this, this->object());
18501860

18511861
statistics_.end_time = uv_hrtime();
Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -878,6 +878,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
878878
// Removes a stream instance from this session
879879
inline void RemoveStream(Http2Stream* stream);
880880

881+
// Indicates whether there currently exist outgoing buffers for this stream.
882+
bool HasWritesOnSocketForStream(Http2Stream* stream);
883+
881884
// Write data to the session
882885
inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs);
883886

Collapse file
+62Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Flags: --expose-gc
2+
'use strict';
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
const makeDuplexPair = require('../common/duplexpair');
9+
10+
// Make sure the Http2Stream destructor works, since we don't clean the
11+
// stream up like we would otherwise do.
12+
process.on('exit', global.gc);
13+
14+
{
15+
const { clientSide, serverSide } = makeDuplexPair();
16+
17+
let serverSideHttp2Stream;
18+
let serverSideHttp2StreamDestroyed = false;
19+
const server = http2.createServer();
20+
server.on('stream', common.mustCall((stream, headers) => {
21+
serverSideHttp2Stream = stream;
22+
stream.respond({
23+
'content-type': 'text/html',
24+
':status': 200
25+
});
26+
27+
const originalWrite = serverSide._write;
28+
serverSide._write = (buf, enc, cb) => {
29+
if (serverSideHttp2StreamDestroyed) {
30+
serverSide.destroy();
31+
serverSide.write = () => {};
32+
} else {
33+
setImmediate(() => {
34+
originalWrite.call(serverSide, buf, enc, () => setImmediate(cb));
35+
});
36+
}
37+
};
38+
39+
// Enough data to fit into a single *session* window,
40+
// not enough data to fit into a single *stream* window.
41+
stream.write(Buffer.alloc(40000));
42+
}));
43+
44+
server.emit('connection', serverSide);
45+
46+
const client = http2.connect('http://localhost:80', {
47+
createConnection: common.mustCall(() => clientSide)
48+
});
49+
50+
const req = client.request({ ':path': '/' });
51+
52+
req.on('response', common.mustCall((headers) => {
53+
assert.strictEqual(headers[':status'], 200);
54+
}));
55+
56+
req.on('data', common.mustCallAtLeast(() => {
57+
if (!serverSideHttp2StreamDestroyed) {
58+
serverSideHttp2Stream.destroy();
59+
serverSideHttp2StreamDestroyed = true;
60+
}
61+
}));
62+
}

0 commit comments

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