http2: safer Http2Session destructor

It's hypothetically (and with certain V8 flags) possible for the session to be garbage collected before all the streams are. In that case, trying to remove the stream from the session will lead to a segfault due to attempting to access no longer valid memory. Fix this by unsetting the session on any streams still around when destroying. PR-URL: nodejs#21194 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
gibfahn · Jun 11, 2018 · 083139d · 083139d
1 parent f86a181
commit 083139d
Showing 1 changed file with 8 additions and 5 deletions.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -499,6 +499,8 @@ Http2Session::Http2Session(Environment* env,
 Http2Session::~Http2Session() {
   CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0);
   Debug(this, "freeing nghttp2 session");
+  for (const auto& stream : streams_)
+    stream.second->session_ = nullptr;
   nghttp2_session_del(session_);
 }
 
@@ -1706,11 +1708,11 @@ Http2Stream::Http2Stream(
 
 
 Http2Stream::~Http2Stream() {
+  if (session_ == nullptr)
+    return;
   Debug(this, "tearing down stream");
-  if (session_ != nullptr) {
-    session_->RemoveStream(this);
-    session_ = nullptr;
-  }
+  session_->RemoveStream(this);
+  session_ = nullptr;
 }
 
 std::string Http2Stream::diagnostic_name() const {
@@ -1785,7 +1787,8 @@ void Http2Stream::Destroy() {
     // 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))
+    if (stream->session() == nullptr ||
+        !stream->session()->HasWritesOnSocketForStream(stream))
       delete stream;
   }, this, this->object());