From 0b3c80ca31db5bbf79e0439304653e14e6302e30 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 27 Jun 2018 12:55:33 +0200
Subject: [PATCH] http2: fix issues with aborted `respondWithFile()`s

PR-URL: https://github.com/nodejs/node/pull/21561
Fixes: https://github.com/nodejs/node/issues/20824
Fixes: https://github.com/nodejs/node/issues/21560
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 src/node_http2.cc                             |  3 +-
 src/stream_pipe.cc                            | 11 ++++--
 src/stream_pipe.h                             | 10 +++--
 ...ttp2-respond-with-file-connection-abort.js | 37 +++++++++++++++++++
 4 files changed, 52 insertions(+), 9 deletions(-)
 create mode 100644 test/parallel/test-http2-respond-with-file-connection-abort.js

diff --git a/src/node_http2.cc b/src/node_http2.cc
index dfe0e4e7ae3e9a..d1319c9d82fd97 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -2060,10 +2060,9 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap,
                          uv_buf_t* bufs,
                          size_t nbufs,
                          uv_stream_t* send_handle) {
-  CHECK(!this->IsDestroyed());
   CHECK_NULL(send_handle);
   Http2Scope h2scope(this);
-  if (!IsWritable()) {
+  if (!IsWritable() || IsDestroyed()) {
     req_wrap->Done(UV_EOF);
     return 0;
   }
diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc
index bfe7d4297257a0..e19f98e35d2821 100644
--- a/src/stream_pipe.cc
+++ b/src/stream_pipe.cc
@@ -57,9 +57,11 @@ void StreamPipe::Unpipe() {
   if (is_closed_)
     return;
 
-  // Note that we cannot use virtual methods on `source` and `sink` here,
-  // because this function can be called from their destructors via
+  // Note that we possibly cannot use virtual methods on `source` and `sink`
+  // here, because this function can be called from their destructors via
   // `OnStreamDestroy()`.
+  if (!source_destroyed_)
+    source()->ReadStop();
 
   is_closed_ = true;
   is_reading_ = false;
@@ -144,7 +146,8 @@ void StreamPipe::ProcessData(size_t nread, const uv_buf_t& buf) {
     is_writing_ = true;
     is_reading_ = false;
     res.wrap->SetAllocatedStorage(buf.base, buf.len);
-    source()->ReadStop();
+    if (source() != nullptr)
+      source()->ReadStop();
   }
 }
 
@@ -183,6 +186,7 @@ void StreamPipe::WritableListener::OnStreamAfterShutdown(ShutdownWrap* w,
 
 void StreamPipe::ReadableListener::OnStreamDestroy() {
   StreamPipe* pipe = ContainerOf(&StreamPipe::readable_listener_, this);
+  pipe->source_destroyed_ = true;
   if (!pipe->is_eof_) {
     OnStreamRead(UV_EPIPE, uv_buf_init(nullptr, 0));
   }
@@ -190,6 +194,7 @@ void StreamPipe::ReadableListener::OnStreamDestroy() {
 
 void StreamPipe::WritableListener::OnStreamDestroy() {
   StreamPipe* pipe = ContainerOf(&StreamPipe::writable_listener_, this);
+  pipe->sink_destroyed_ = true;
   pipe->is_eof_ = true;
   pipe->Unpipe();
 }
diff --git a/src/stream_pipe.h b/src/stream_pipe.h
index b72a60941b610a..36a0b1dc08106b 100644
--- a/src/stream_pipe.h
+++ b/src/stream_pipe.h
@@ -23,16 +23,18 @@ class StreamPipe : public AsyncWrap {
   }
 
  private:
-  StreamBase* source();
-  StreamBase* sink();
+  inline StreamBase* source();
+  inline StreamBase* sink();
 
-  void ShutdownWritable();
-  void FlushToWritable();
+  inline void ShutdownWritable();
+  inline void FlushToWritable();
 
   bool is_reading_ = false;
   bool is_writing_ = false;
   bool is_eof_ = false;
   bool is_closed_ = true;
+  bool sink_destroyed_ = false;
+  bool source_destroyed_ = false;
 
   // Set a default value so that when we’re coming from Start(), we know
   // that we don’t want to read just yet.
diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js
new file mode 100644
index 00000000000000..25926b2c9805a3
--- /dev/null
+++ b/test/parallel/test-http2-respond-with-file-connection-abort.js
@@ -0,0 +1,37 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const net = require('net');
+
+const {
+  HTTP2_HEADER_CONTENT_TYPE
+} = http2.constants;
+
+const server = http2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  stream.respondWithFile(process.execPath, {
+    [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream'
+  });
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall(() => {}));
+  req.on('data', common.mustCall(() => {
+    net.Socket.prototype.destroy.call(client.socket);
+    server.close();
+  }));
+  req.end();
+}));
+
+// TODO(addaleax): This is a *hack*. HTTP/2 needs to have a proper way of
+// dealing with this kind of issue.
+process.once('uncaughtException', (err) => {
+  if (err.code === 'ECONNRESET') return;
+  throw err;
+});