From b11e19e8eed059ddf473657c28c357987ca0015e Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 17 May 2018 23:03:15 +0400 Subject: http2: fix several serious bugs Currently http2 does not properly submit GOAWAY frames when a session is being destroyed. It also doesn't properly handle when the other party severs the connection after sending a GOAWAY frame, even though it should. Edge, IE & Safari are currently unable to handle empty TRAILERS frames despite them being correctly to spec. Instead send an empty DATA frame with END_STREAM flag in those situations. Fix and adjust several flaky and/or incorrect tests. PR-URL: https://github.com/nodejs/node/pull/20772 Fixes: https://github.com/nodejs/node/issues/20705 Fixes: https://github.com/nodejs/node/issues/20750 Fixes: https://github.com/nodejs/node/issues/20850 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/node_http2.cc | 50 +++++++++++++++++++++++++++++++++----------------- src/node_http2.h | 8 ++++++-- 2 files changed, 39 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/node_http2.cc b/src/node_http2.cc index 94f774e2d3..4745707f9a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -577,9 +577,9 @@ void Http2Session::EmitStatistics() { void Http2Session::Close(uint32_t code, bool socket_closed) { DEBUG_HTTP2SESSION(this, "closing session"); - if (flags_ & SESSION_STATE_CLOSED) + if (flags_ & SESSION_STATE_CLOSING) return; - flags_ |= SESSION_STATE_CLOSED; + flags_ |= SESSION_STATE_CLOSING; // Stop reading on the i/o stream if (stream_ != nullptr) @@ -587,16 +587,18 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // If the socket is not closed, then attempt to send a closing GOAWAY // frame. There is no guarantee that this GOAWAY will be received by - // the peer but the HTTP/2 spec recommends sendinng it anyway. We'll + // the peer but the HTTP/2 spec recommends sending it anyway. We'll // make a best effort. if (!socket_closed) { - Http2Scope h2scope(this); DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code); CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0); + SendPendingData(); } else if (stream_ != nullptr) { stream_->RemoveStreamListener(this); } + flags_ |= SESSION_STATE_CLOSED; + // If there are outstanding pings, those will need to be canceled, do // so on the next iteration of the event loop to avoid calling out into // javascript since this may be called during garbage collection. @@ -1355,25 +1357,32 @@ void Http2Session::MaybeScheduleWrite() { } } +void Http2Session::MaybeStopReading() { + int want_read = nghttp2_session_want_read(session_); + DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read); + if (want_read == 0) + stream_->ReadStop(); +} + // Unset the sending state, finish up all current writes, and reset // storage for data and metadata that was associated with these writes. void Http2Session::ClearOutgoing(int status) { CHECK_NE(flags_ & SESSION_STATE_SENDING, 0); + flags_ &= ~SESSION_STATE_SENDING; + if (outgoing_buffers_.size() > 0) { outgoing_storage_.clear(); - for (const nghttp2_stream_write& wr : outgoing_buffers_) { + std::vector current_outgoing_buffers_; + current_outgoing_buffers_.swap(outgoing_buffers_); + for (const nghttp2_stream_write& wr : current_outgoing_buffers_) { WriteWrap* wrap = wr.req_wrap; if (wrap != nullptr) wrap->Done(status); } - - outgoing_buffers_.clear(); } - flags_ &= ~SESSION_STATE_SENDING; - // Now that we've finished sending queued data, if there are any pending // RstStreams we should try sending again and then flush them one by one. if (pending_rst_streams_.size() > 0) { @@ -1484,8 +1493,7 @@ uint8_t Http2Session::SendPendingData() { ClearOutgoing(res.err); } - DEBUG_HTTP2SESSION2(this, "wants data in return? %d", - nghttp2_session_want_read(session_)); + MaybeStopReading(); return 0; } @@ -1618,8 +1626,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { }; MakeCallback(env()->error_string(), arraysize(argv), argv); } else { - DEBUG_HTTP2SESSION2(this, "processed %d bytes. wants more? %d", ret, - nghttp2_session_want_read(session_)); + MaybeStopReading(); } } @@ -1814,6 +1821,7 @@ void Http2Stream::OnTrailers() { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); + flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS; MakeCallback(env()->ontrailers_string(), 0, nullptr); } @@ -1822,7 +1830,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending %d trailers", len); - int ret = nghttp2_submit_trailer(**session_, id_, nva, len); + int ret; + // Sending an empty trailers frame poses problems in Safari, Edge & IE. + // Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM + // to indicate that the stream is ready to be closed. + if (len == 0) { + Http2Stream::Provider::Stream prov(this, 0); + ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov); + } else { + ret = nghttp2_submit_trailer(**session_, id_, nva, len); + } CHECK_NE(ret, NGHTTP2_ERR_NOMEM); return ret; } @@ -2351,8 +2368,7 @@ void Http2Stream::Info(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length())); - DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", - headers->Length()); + DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length()); } // Submits trailing headers on the Http2Stream @@ -2367,7 +2383,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length())); - DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length()); + DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length()); } // Grab the numeric id of the Http2Stream diff --git a/src/node_http2.h b/src/node_http2.h index f473e30286..043449624f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -433,7 +433,8 @@ enum session_state_flags { SESSION_STATE_HAS_SCOPE = 0x1, SESSION_STATE_WRITE_SCHEDULED = 0x2, SESSION_STATE_CLOSED = 0x4, - SESSION_STATE_SENDING = 0x8, + SESSION_STATE_CLOSING = 0x8, + SESSION_STATE_SENDING = 0x10, }; // This allows for 4 default-sized frames with their frame headers @@ -619,7 +620,7 @@ class Http2Stream : public AsyncWrap, inline bool IsClosed() const { return flags_ & NGHTTP2_STREAM_FLAG_CLOSED; - } + } inline bool HasTrailers() const { return flags_ & NGHTTP2_STREAM_FLAG_TRAILERS; @@ -827,6 +828,9 @@ class Http2Session : public AsyncWrap, public StreamListener { // Schedule a write if nghttp2 indicates it wants to write to the socket. void MaybeScheduleWrite(); + // Stop reading if nghttp2 doesn't want to anymore. + void MaybeStopReading(); + // Returns pointer to the stream, or nullptr if stream does not exist inline Http2Stream* FindStream(int32_t id); -- cgit v1.2.3