Skip to content

Commit 083139d

Browse files
committed
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: #21194 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
1 parent f86a181 commit 083139d

File tree

1 file changed

+8
-5
lines changed

1 file changed

+8
-5
lines changed

Diff for: src/node_http2.cc

+8-5
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,8 @@ Http2Session::Http2Session(Environment* env,
499499
Http2Session::~Http2Session() {
500500
CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0);
501501
Debug(this, "freeing nghttp2 session");
502+
for (const auto& stream : streams_)
503+
stream.second->session_ = nullptr;
502504
nghttp2_session_del(session_);
503505
}
504506

@@ -1706,11 +1708,11 @@ Http2Stream::Http2Stream(
17061708

17071709

17081710
Http2Stream::~Http2Stream() {
1711+
if (session_ == nullptr)
1712+
return;
17091713
Debug(this, "tearing down stream");
1710-
if (session_ != nullptr) {
1711-
session_->RemoveStream(this);
1712-
session_ = nullptr;
1713-
}
1714+
session_->RemoveStream(this);
1715+
session_ = nullptr;
17141716
}
17151717

17161718
std::string Http2Stream::diagnostic_name() const {
@@ -1785,7 +1787,8 @@ void Http2Stream::Destroy() {
17851787
// We can destroy the stream now if there are no writes for it
17861788
// already on the socket. Otherwise, we'll wait for the garbage collector
17871789
// to take care of cleaning up.
1788-
if (!stream->session()->HasWritesOnSocketForStream(stream))
1790+
if (stream->session() == nullptr ||
1791+
!stream->session()->HasWritesOnSocketForStream(stream))
17891792
delete stream;
17901793
}, this, this->object());
17911794

0 commit comments

Comments
 (0)