From 2c16d3c592cc72e9ccd497af03cb9b5e6faec32f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 5 Jun 2018 20:34:47 -0400 Subject: [PATCH 1/4] 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. --- src/node_http2.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 5629298eb94a20..500ff275804968 100644 --- 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 (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 { From f602aabdf929e94d8a93057e61e8d1a878e6ce10 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 6 Jun 2018 09:12:39 -0400 Subject: [PATCH 2/4] src: fix http2 typos --- src/node_http2.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 500ff275804968..7165d6752e1ef3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -521,7 +521,7 @@ void Http2Stream::EmitStatistics() { Http2StreamPerformanceEntry* entry = new Http2StreamPerformanceEntry(env(), id_, statistics_); env()->SetImmediate([](Environment* env, void* data) { - // This takes ownership, the entr is destroyed at the end of this scope. + // This takes ownership, the entry is destroyed at the end of this scope. std::unique_ptr entry { static_cast(data) }; if (!HasHttp2Observer(env)) @@ -998,7 +998,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // `buf.base == nullptr` is the default Http2StreamListener's way // of saying that it wants a pointer to the raw original. // Since it has access to the original socket buffer from which the data - // was read in the first place, it can use that to minizime ArrayBuffer + // was read in the first place, it can use that to minimize ArrayBuffer // allocations. if (LIKELY(buf.base == nullptr)) buf.base = reinterpret_cast(const_cast(data)); @@ -1338,7 +1338,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { } // If the underlying nghttp2_session struct has data pending in its outbound -// queue, MaybeScheduleWrite will schedule a SendPendingData() call to occcur +// queue, MaybeScheduleWrite will schedule a SendPendingData() call to occur // on the next iteration of the Node.js event loop (using the SetImmediate // queue), but only if a write has not already been scheduled. void Http2Session::MaybeScheduleWrite() { @@ -2179,7 +2179,7 @@ void Http2Session::RefreshSettings(const FunctionCallbackInfo& args) { // A TypedArray instance is shared between C++ and JS land to contain state // information of the current Http2Session. This updates the values in the -// TypedRray so those can be read in JS land. +// TypedArray so those can be read in JS land. void Http2Session::RefreshState(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; @@ -2512,7 +2512,7 @@ void Http2Session::AltSvc(int32_t id, origin, origin_len, value, value_len), 0); } -// Submits an AltSvc frame to the sent to the connected peer. +// Submits an AltSvc frame to be sent to the connected peer. void Http2Session::AltSvc(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; From 7abe1d85ce99ad9aa1db352fee6a519a3a13ff0f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 7 Jun 2018 13:07:59 -0400 Subject: [PATCH 3/4] fixup: another segfault --- src/node_http2.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7165d6752e1ef3..534fc845bb60b9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1787,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()); From 4c9aa9c18bed3bfd55c18860769497dcf2e65bdd Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 7 Jun 2018 16:45:59 -0400 Subject: [PATCH 4/4] fixup: addaleax feedback --- src/node_http2.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 534fc845bb60b9..9388e6ef6609b9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -499,7 +499,7 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); Debug(this, "freeing nghttp2 session"); - for (auto stream : streams_) + for (const auto& stream : streams_) stream.second->session_ = nullptr; nghttp2_session_del(session_); }