http2: force through RST_STREAM in destroy

apapirovski · BethGriggs · commit b22266cc978b · 2018-10-17T00:07:24.000+01:00
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>
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -286,7 +286,7 @@ function onStreamClose(code) {
         `${sessionName(stream[kSession][kType])}]: closed with code ${code}`);
 
   if (!stream.closed)
-    closeStream(stream, code, false);
+    closeStream(stream, code, kNoRstStream);
 
   if (stream[kState].fd !== undefined)
     tryClose(stream[kState].fd);
@@ -1458,7 +1458,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;
@@ -1481,9 +1485,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);
@@ -1845,7 +1850,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
@@ -1839,6 +1839,8 @@ inline 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_HTTP2STREAM(this, "destroying stream");
diff --git a/src/node_http2.h b/src/node_http2.h
@@ -9,6 +9,7 @@
 #include "stream_base-inl.h"
 #include "string_bytes.h"
 
+#include <algorithm>
 #include <queue>
 
 namespace node {
@@ -880,6 +881,12 @@ class Http2Session : public AsyncWrap {
     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);
+  }
+
   static void OnStreamAllocImpl(size_t suggested_size,
                                 uv_buf_t* buf,
                                 void* ctx);
diff --git a/test/parallel/test-http2-large-write-destroy.js 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();
+  }));
+}));
