Skip to content

Commit dee250f

Browse files
apapirovskitargos
authored andcommittedJun 13, 2018
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 688bdfe commit dee250f

File tree

1 file changed

+8
-7
lines changed

1 file changed

+8
-7
lines changed
 

‎src/node_http2.cc

+8-7
Original file line numberDiff line numberDiff line change
@@ -499,8 +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& iter : streams_)
503-
iter.second->session_ = nullptr;
502+
for (const auto& stream : streams_)
503+
stream.second->session_ = nullptr;
504504
nghttp2_session_del(session_);
505505
}
506506

@@ -1710,11 +1710,11 @@ Http2Stream::Http2Stream(
17101710

17111711

17121712
Http2Stream::~Http2Stream() {
1713+
if (session_ == nullptr)
1714+
return;
17131715
Debug(this, "tearing down stream");
1714-
if (session_ != nullptr) {
1715-
session_->RemoveStream(this);
1716-
session_ = nullptr;
1717-
}
1716+
session_->RemoveStream(this);
1717+
session_ = nullptr;
17181718
}
17191719

17201720
std::string Http2Stream::diagnostic_name() const {
@@ -1789,7 +1789,8 @@ void Http2Stream::Destroy() {
17891789
// We can destroy the stream now if there are no writes for it
17901790
// already on the socket. Otherwise, we'll wait for the garbage collector
17911791
// to take care of cleaning up.
1792-
if (!stream->session()->HasWritesOnSocketForStream(stream))
1792+
if (stream->session() == nullptr ||
1793+
!stream->session()->HasWritesOnSocketForStream(stream))
17931794
delete stream;
17941795
}, this, this->object());
17951796

0 commit comments

Comments
 (0)