Skip to content

Commit

Permalink
http2: safer Http2Session destructor
Browse files Browse the repository at this point in the history
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.

Backport-PR-URL: #22850
PR-URL: #21194
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
  • Loading branch information
apapirovski authored and BethGriggs committed Oct 16, 2018
1 parent 3c8c53f commit 0a8d086
Showing 1 changed file with 9 additions and 8 deletions.
17 changes: 9 additions & 8 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,8 @@ Http2Session::~Http2Session() {
ClearWrap(object());
persistent().Reset();
CHECK(persistent().IsEmpty());
for (const auto& iter : streams_)
iter.second->session_ = nullptr;
for (const auto& stream : streams_)
stream.second->session_ = nullptr;
Unconsume();
DEBUG_HTTP2SESSION(this, "freeing nghttp2 session");
nghttp2_session_del(session_);
Expand Down Expand Up @@ -1782,11 +1782,11 @@ Http2Stream::Http2Stream(


Http2Stream::~Http2Stream() {
if (session_ != nullptr) {
DEBUG_HTTP2STREAM(this, "tearing down stream");
session_->RemoveStream(this);
session_ = nullptr;
}
if (session_ == nullptr)
return;
DEBUG_HTTP2STREAM(this, "tearing down stream");
session_->RemoveStream(this);
session_ = nullptr;

persistent().Reset();
CHECK(persistent().IsEmpty());
Expand Down Expand Up @@ -1862,7 +1862,8 @@ inline 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());

Expand Down

0 comments on commit 0a8d086

Please sign in to comment.