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.
Backport-PR-URL: #22850
PR-URL: #20772Fixes: #20705Fixes: #20750Fixes: #20850
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: apapirovski

doc/api/http2.md

@@ -884,6 +884,10 @@ The `'trailers'` event is emitted when a block of headers associated with
 trailing header fields is received. The listener callback is passed the
 [HTTP/2 Headers Object][] and flags associated with the headers.
 
+Note that this event might not be emitted if `http2stream.end()` is called
+before trailers are received and the incoming data is not being read or
+listened for.
+
 ```js
 stream.on('trailers', (headers, flags) => {
 console.log(headers);

lib/internal/http2/core.js

@@ -292,20 +292,25 @@ function onStreamClose(code) {
 tryClose(stream[kState].fd);
 
 // Defer destroy we actually emit end.
-if(stream._readableState.endEmitted||code!==NGHTTP2_NO_ERROR){
+if(!stream.readable||code!==NGHTTP2_NO_ERROR){
 // If errored or ended, we can destroy immediately.
-stream[kMaybeDestroy](null,code);
+stream[kMaybeDestroy](code);
 }else{
 // Wait for end to destroy.
 stream.on('end',stream[kMaybeDestroy]);
 // Push a null so the stream can end whenever the client consumes
 // it completely.
 stream.push(null);
-// If the client hasn't tried to consume the stream and there is no
-// resume scheduled (which would indicate they would consume in the future),
-// then just dump the incoming data so that the stream can be destroyed.
-if(!stream[kState].didRead&&!stream._readableState.resumeScheduled)
+
+// If the user hasn't tried to consume the stream (and this is a server
+// session) then just dump the incoming data so that the stream can
+// be destroyed.
+if(stream[kSession][kType]===NGHTTP2_SESSION_SERVER&&
+!stream[kState].didRead&&
+stream._readableState.flowing===null)
 stream.resume();
+else
+stream.read(0);
 }
 }
 
@@ -330,7 +335,7 @@ function onStreamRead(nread, buf) {
 `${sessionName(stream[kSession][kType])}]: ending readable.`);
 
 // defer this until we actually emit end
-if(stream._readableState.endEmitted){
+if(!stream.readable){
 stream[kMaybeDestroy]();
 }else{
 stream.on('end',stream[kMaybeDestroy]);
@@ -421,7 +426,7 @@ function onGoawayData(code, lastStreamID, buf) {
 // condition on this side of the session that caused the
 // shutdown.
 session.destroy(newerrors.Error('ERR_HTTP2_SESSION_ERROR',code),
-{errorCode: NGHTTP2_NO_ERROR});
+NGHTTP2_NO_ERROR);
 }
 }
 
@@ -772,6 +777,21 @@ function emitClose(self, error) {
 self.emit('close');
 }
 
+functionfinishSessionDestroy(session,error){
+constsocket=session[kSocket];
+if(!socket.destroyed)
+socket.destroy(error);
+
+session[kProxySocket]=undefined;
+session[kSocket]=undefined;
+session[kHandle]=undefined;
+socket[kSession]=undefined;
+socket[kServer]=undefined;
+
+// Finally, emit the close and error events (if necessary) on next tick.
+process.nextTick(emitClose,session,error);
+}
+
 // Upon creation, the Http2Session takes ownership of the socket. The session
 // may not be ready to use immediately if the socket is not yet fully connected.
 // In that case, the Http2Session will wait for the socket to connect. Once
@@ -828,6 +848,8 @@ class Http2Session extends EventEmitter {
 
 this[kState]={
 flags: SESSION_FLAGS_PENDING,
+goawayCode: null,
+goawayLastStreamID: null,
 streams: newMap(),
 pendingStreams: newSet(),
 pendingAck: 0,
@@ -1130,25 +1152,13 @@ class Http2Session extends EventEmitter {
 if(handle!==undefined)
 handle.destroy(code,socket.destroyed);
 
-// If there is no error, use setImmediate to destroy the socket on the
+// If the socket is alive, use setImmediate to destroy the session on the
 // next iteration of the event loop in order to give data time to transmit.
 // Otherwise, destroy immediately.
-if(!socket.destroyed){
-if(!error){
-setImmediate(socket.destroy.bind(socket));
-}else{
-socket.destroy(error);
-}
-}
-
-this[kProxySocket]=undefined;
-this[kSocket]=undefined;
-this[kHandle]=undefined;
-socket[kSession]=undefined;
-socket[kServer]=undefined;
-
-// Finally, emit the close and error events (if necessary) on next tick.
-process.nextTick(emitClose,this,error);
+if(!socket.destroyed)
+setImmediate(finishSessionDestroy,this,error);
+else
+finishSessionDestroy(this,error);
 }
 
 // Closing the session will:
@@ -1422,11 +1432,8 @@ function afterDoStreamWrite(status, handle, req) {
 }
 
 functionstreamOnResume(){
-if(!this.destroyed&&!this.pending){
-if(!this[kState].didRead)
-this[kState].didRead=true;
+if(!this.destroyed)
 this[kHandle].readStart();
-}
 }
 
 functionstreamOnPause(){
@@ -1441,6 +1448,16 @@ function afterShutdown() {
 stream[kMaybeDestroy]();
 }
 
+functionfinishSendTrailers(stream,headersList){
+stream[kState].flags&=~STREAM_FLAGS_HAS_TRAILERS;
+
+constret=stream[kHandle].trailers(headersList);
+if(ret<0)
+stream.destroy(newNghttpError(ret));
+else
+stream[kMaybeDestroy]();
+}
+
 functioncloseStream(stream,code,shouldSubmitRstStream=true){
 conststate=stream[kState];
 state.flags|=STREAM_FLAGS_CLOSED;
@@ -1502,6 +1519,10 @@ class Http2Stream extends Duplex {
 this[kSession]=session;
 session[kState].pendingStreams.add(this);
 
+// Allow our logic for determining whether any reads have happened to
+// work in all situations. This is similar to what we do in _http_incoming.
+this._readableState.readingMore=true;
+
 this[kState]={
 didRead: false,
 flags: STREAM_FLAGS_PENDING,
@@ -1510,7 +1531,6 @@ class Http2Stream extends Duplex {
 trailersReady: false
 };
 
-this.on('resume',streamOnResume);
 this.on('pause',streamOnPause);
 }
 
@@ -1717,6 +1737,10 @@ class Http2Stream extends Duplex {
 this.push(null);
 return;
 }
+if(!this[kState].didRead){
+this._readableState.readingMore=false;
+this[kState].didRead=true;
+}
 if(!this.pending){
 streamOnResume.call(this);
 }else{
@@ -1765,13 +1789,8 @@ class Http2Stream extends Duplex {
 throwheadersList;
 this[kSentTrailers]=headers;
 
-this[kState].flags&=~STREAM_FLAGS_HAS_TRAILERS;
-
-constret=this[kHandle].trailers(headersList);
-if(ret<0)
-this.destroy(newNghttpError(ret));
-else
-this[kMaybeDestroy]();
+// Send the trailers in setImmediate so we don't do it on nghttp2 stack.
+setImmediate(finishSendTrailers,this,headersList);
 }
 
 getclosed(){
@@ -1861,15 +1880,15 @@ class Http2Stream extends Duplex {
 }
 // The Http2Stream can be destroyed if it has closed and if the readable
 // side has received the final chunk.
-[kMaybeDestroy](error,code=NGHTTP2_NO_ERROR){
-if(error||code!==NGHTTP2_NO_ERROR){
-this.destroy(error);
+[kMaybeDestroy](code=NGHTTP2_NO_ERROR){
+if(code!==NGHTTP2_NO_ERROR){
+this.destroy();
 return;
 }
 
 // TODO(mcollina): remove usage of _*State properties
-if(this._writableState.ended&&this._writableState.pendingcb===0){
-if(this._readableState.ended&&this.closed){
+if(!this.writable){
+if(!this.readable&&this.closed){
 this.destroy();
 return;
 }
@@ -1882,7 +1901,7 @@ class Http2Stream extends Duplex {
 this[kSession][kType]===NGHTTP2_SESSION_SERVER&&
 !(state.flags&STREAM_FLAGS_HAS_TRAILERS)&&
 !state.didRead&&
-!this._readableState.resumeScheduled){
+this._readableState.flowing===null){
 this.close();
 }
 }
@@ -2445,6 +2464,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
 functionsocketOnError(error){
 constsession=this[kSession];
 if(session!==undefined){
+// We can ignore ECONNRESET after GOAWAY was received as there's nothing
+// we can do and the other side is fully within its rights to do so.
+if(error.code==='ECONNRESET'&&session[kState].goawayCode!==null)
+returnsession.destroy();
 debug(`Http2Session ${sessionName(session[kType])}: socket error [`+
 `${error.message}]`);
 session.destroy(error);

src/node_http2.cc

@@ -631,26 +631,28 @@ inline void Http2Session::EmitStatistics() {
 voidHttp2Session::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)
  stream_->ReadStop();
 
 // 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 {
 Unconsume();
  }
 
+ 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.
@@ -1387,25 +1389,32 @@ void Http2Session::MaybeScheduleWrite() {
  }
 }
 
+voidHttp2Session::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.
 voidHttp2Session::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<nghttp2_stream_write> 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) {
@@ -1525,8 +1534,7 @@ uint8_t Http2Session::SendPendingData() {
  req->Dispose();
  }
 
-DEBUG_HTTP2SESSION2(this, "wants data in return? %d",
-nghttp2_session_want_read(session_));
+MaybeStopReading();
 
 return0;
 }
@@ -1691,8 +1699,7 @@ void Http2Session::OnStreamReadImpl(ssize_t nread,
  };
  session->MakeCallback(env->error_string(), arraysize(argv), argv);
  } else {
-DEBUG_HTTP2SESSION2(session, "processed %d bytes. wants more? %d", ret,
-nghttp2_session_want_read(**session));
+ session->MaybeStopReading();
  }
  }
 
@@ -1928,6 +1935,7 @@ void Http2Stream::OnTrailers() {
  HandleScope scope(isolate);
  Local<Context> context = env()->context();
  Context::Scope context_scope(context);
+ flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
 MakeCallback(env()->ontrailers_string(), 0, nullptr);
 }
 
@@ -1936,7 +1944,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;
 }
@@ -2562,8 +2579,7 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& 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
@@ -2578,7 +2594,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& 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