diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7e2faa97bf02da..97a6a5b4f1f89c 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -337,7 +337,7 @@ function onStreamClose(code) { `${sessionName(stream[kSession][kType])}]: closed with code ${code}`); if (!stream.closed) - closeStream(stream, code, false); + closeStream(stream, code, kNoRstStream); stream[kState].fd = -1; // Defer destroy we actually emit end. @@ -1476,7 +1476,11 @@ function finishSendTrailers(stream, headersList) { stream[kMaybeDestroy](); } -function closeStream(stream, code, shouldSubmitRstStream = true) { +const kNoRstStream = 0; +const kSubmitRstStream = 1; +const kForceRstStream = 2; + +function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { const state = stream[kState]; state.flags |= STREAM_FLAGS_CLOSED; state.rstCode = code; @@ -1499,9 +1503,10 @@ function closeStream(stream, code, shouldSubmitRstStream = true) { stream.end(); } - if (shouldSubmitRstStream) { + if (rstStreamStatus !== kNoRstStream) { const finishFn = finishCloseStream.bind(stream, code); - if (!ending || finished || code !== NGHTTP2_NO_ERROR) + if (!ending || finished || code !== NGHTTP2_NO_ERROR || + rstStreamStatus === kForceRstStream) finishFn(); else stream.once('finish', finishFn); @@ -1852,7 +1857,7 @@ class Http2Stream extends Duplex { const hasHandle = handle !== undefined; if (!this.closed) - closeStream(this, code, hasHandle); + closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream); this.push(null); if (hasHandle) { diff --git a/src/node_http2.cc b/src/node_http2.cc index 0bfdec9a57e380..be43cc1e69067b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1762,6 +1762,8 @@ void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed if (IsDestroyed()) return; + if (session_->HasPendingRstStream(id_)) + FlushRstStream(); flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED; Debug(this, "destroying stream"); diff --git a/src/node_http2.h b/src/node_http2.h index 824084d0bafdaf..e0a93c8f0fdc46 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -9,6 +9,7 @@ #include "stream_base-inl.h" #include "string_bytes.h" +#include #include namespace node { @@ -803,6 +804,12 @@ class Http2Session : public AsyncWrap, public StreamListener { pending_rst_streams_.emplace_back(stream_id); } + inline bool HasPendingRstStream(int32_t stream_id) { + return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(), + pending_rst_streams_.end(), + stream_id); + } + // Handle reads/writes from the underlying network transport. void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override; void OnStreamAfterWrite(WriteWrap* w, int status) override; diff --git a/test/parallel/test-http2-large-write-destroy.js b/test/parallel/test-http2-large-write-destroy.js new file mode 100644 index 00000000000000..24c0a055cc943f --- /dev/null +++ b/test/parallel/test-http2-large-write-destroy.js @@ -0,0 +1,40 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); +const http2 = require('http2'); + +// This test will result in a crash due to a missed CHECK in C++ or +// a straight-up segfault if the C++ doesn't send RST_STREAM through +// properly when calling destroy. + +const content = Buffer.alloc(60000, 0x44); + +const server = http2.createSecureServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}); +server.on('stream', common.mustCall((stream) => { + stream.respond({ + 'Content-Type': 'application/octet-stream', + 'Content-Length': (content.length.toString() * 2), + 'Vary': 'Accept-Encoding' + }, { waitForTrailers: true }); + + stream.write(content); + stream.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`https://localhost:${server.address().port}`, + { rejectUnauthorized: false }); + + const req = client.request({ ':path': '/' }); + req.end(); + + req.on('close', common.mustCall(() => { + client.close(); + server.close(); + })); +}));