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.

PR-URL: https://github.com/nodejs/node/pull/19002
Fixes: https://github.com/nodejs/node/issues/18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

1 files changed, 15 insertions, 5 deletions

diff --git a/src/node_http2.cc b/src/node_http2.cc
index f1454aa11e..7650969f86 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1700,6 +1700,14 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   stream_buf_ = uv_buf_init(nullptr, 0);
 }
 
+bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) {
+  for (const nghttp2_stream_write& wr : outgoing_buffers_) {
+    if (wr.req_wrap != nullptr && wr.req_wrap->stream() == stream)
+      return true;
+  }
+  return false;
+}
+
 // Every Http2Session session is tightly bound to a single i/o StreamBase
 // (typically a net.Socket or tls.TLSSocket). The lifecycle of the two is
 // tightly coupled with all data transfer between the two happening at the
@@ -1753,13 +1761,11 @@ Http2Stream::Http2Stream(
 
 
 Http2Stream::~Http2Stream() {
+  DEBUG_HTTP2STREAM(this, "tearing down stream");
   if (session_ != nullptr) {
     session_->RemoveStream(this);
     session_ = nullptr;
   }
-
-  if (!object().IsEmpty())
-    ClearWrap(object());
 }
 
 // Notify the Http2Stream that a new block of HEADERS is being processed.
@@ -1837,7 +1843,7 @@ inline void Http2Stream::Destroy() {
     Http2Stream* stream = static_cast<Http2Stream*>(data);
     // Free any remaining outgoing data chunks here. This should be done
     // here because it's possible for destroy to have been called while
-    // we still have qeueued outbound writes.
+    // we still have queued outbound writes.
     while (!stream->queue_.empty()) {
       nghttp2_stream_write& head = stream->queue_.front();
       if (head.req_wrap != nullptr)
@@ -1845,7 +1851,11 @@ inline void Http2Stream::Destroy() {
       stream->queue_.pop();
     }
 
-    delete stream;
+    // We can destroy the stream now if there are no writes for it
+    // already on the socket. Otherwise, we'll wait for the garbage collector
+    // to take care of cleaning up.
+    if (!stream->session()->HasWritesOnSocketForStream(stream))
+      delete stream;
   }, this, this->object());
 
   statistics_.end_time = uv_hrtime();