src: fix HTTP2 mem leak on premature close and ERR_PROTO

This commit fixes a memory leak when the socket is
suddenly closed by the peer (without GOAWAY notification)
and when invalid header (by nghttp2) is identified and the
connection is terminated by peer.

Refs: https://hackerone.com/reports/2841362

Author: RafaelGSS

lib/internal/http2/core.js

@@ -607,11 +607,20 @@ function onFrameError(id, type, code) {
     return;
   debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d',
                   type, id, code);
-  const emitter = session[kState].streams.get(id) || session;
+
+  const stream = session[kState].streams.get(id);
+  const emitter = stream || session;
   emitter[kUpdateTimer]();
   emitter.emit('frameError', type, code, id);
-  session[kState].streams.get(id).close(code);
-  session.close();
+
+  // When a frameError happens is not uncommon that a pending GOAWAY
+  // package from nghttp2 is on flight with a correct error code.
+  // We schedule it using setImmediate to give some time for that
+  // package to arrive.
+  setImmediate(() => {
+    stream?.close(code);
+    session.close();
+  });
 }
 
 function onAltSvc(stream, origin, alt) {

src/node_http2.cc

@@ -844,6 +844,7 @@ bool Http2Session::CanAddStream() {
 }
 
 void Http2Session::AddStream(Http2Stream* stream) {
+  Debug(this, "Adding stream: %d", stream->id());
   CHECK_GE(++statistics_.stream_count, 0);
   streams_[stream->id()] = BaseObjectPtr<Http2Stream>(stream);
   size_t size = streams_.size();
@@ -854,6 +855,7 @@ void Http2Session::AddStream(Http2Stream* stream) {
 
 
 BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
+  Debug(this, "Removing stream: %d", id);
   BaseObjectPtr<Http2Stream> stream;
   if (streams_.empty())
     return stream;
@@ -1030,6 +1032,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle,
   if (UNLIKELY(!stream))
     return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
 
+  Debug(session, "handling header key/pair for stream %d", id);
   // If the stream has already been destroyed, ignore.
   if (!stream->is_destroyed() && !stream->AddHeader(name, value, flags)) {
     // This will only happen if the connected peer sends us more
@@ -1099,9 +1102,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
     return 1;
   }
 
-  // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
+  // If the error is fatal or if error code is one of the following
+  // we emit and error:
+  //
+  // ERR_STREAM_CLOSED: An invalid frame has been received in a closed stream.
+  //
+  // ERR_PROTO: The RFC 7540 specifies:
+  // "An endpoint that encounters a connection error SHOULD first send a GOAWAY
+  // frame (Section 6.8) with the stream identifier of the last stream that it
+  // successfully received from its peer.
+  // The GOAWAY frame includes an error code that indicates the type of error"
+  // The GOAWAY frame is already sent by nghttp2. We emit the error
+  // to liberate the Http2Session to destroy.
   if (nghttp2_is_fatal(lib_error_code) ||
-      lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) {
+      lib_error_code == NGHTTP2_ERR_STREAM_CLOSED ||
+      lib_error_code == NGHTTP2_ERR_PROTO) {
     Environment* env = session->env();
     Isolate* isolate = env->isolate();
     HandleScope scope(isolate);
@@ -1164,7 +1179,6 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
   Debug(session, "frame type %d was not sent, code: %d",
         frame->hd.type, error_code);
 
-  // Do not report if the frame was not sent due to the session closing
   if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
       error_code == NGHTTP2_ERR_STREAM_CLOSED ||
       error_code == NGHTTP2_ERR_STREAM_CLOSING) {
@@ -1173,7 +1187,15 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
     // to destroy the session completely.
     // Further information see: https://github.com/nodejs/node/issues/35233
     session->DecrefHeaders(frame);
-    return 0;
+    // Currently, nghttp2 doesn't not inform us when is the best
+    // time to call session.close(). It relies on a closing connection
+    // from peer. If that doesn't happen, the nghttp2_session will be
+    // closed but the Http2Session will still be up causing a memory leak.
+    // Therefore, if the GOAWAY frame couldn't be send due to
+    // ERR_SESSION_CLOSING we should force close from our side.
+    if (frame->hd.type != 0x03) {
+      return 0;
+    }
   }
 
   Isolate* isolate = env->isolate();
@@ -1239,12 +1261,15 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
 // ignore these. If this callback was not provided, nghttp2 would handle
 // invalid headers strictly and would shut down the stream. We are intentionally
 // being more lenient here although we may want to revisit this choice later.
-int Http2Session::OnInvalidHeader(nghttp2_session* session,
+int Http2Session::OnInvalidHeader(nghttp2_session* handle,
                                   const nghttp2_frame* frame,
                                   nghttp2_rcbuf* name,
                                   nghttp2_rcbuf* value,
                                   uint8_t flags,
                                   void* user_data) {
+  Http2Session* session = static_cast<Http2Session*>(user_data);
+  int32_t id = GetFrameID(frame);
+  Debug(session, "invalid header received for stream %d", id);
   // Ignore invalid header fields by default.
   return 0;
 }
@@ -1638,6 +1663,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
 
 // Called by OnFrameReceived when a complete SETTINGS frame has been received.
 void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
+  Debug(this, "handling settings frame");
   bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
   if (!ack) {
     js_fields_->bitfield &= ~(1 << kSessionRemoteSettingsIsUpToDate);

test/parallel/test-http2-connect-method-extended-cant-turn-off.js

@@ -27,4 +27,10 @@ server.listen(0, common.mustCall(() => {
       server.close();
     }));
   }));
+
+  client.on('error', common.expectsError({
+    code: 'ERR_HTTP2_ERROR',
+    name: 'Error',
+    message: 'Protocol error'
+  }));
 }));

test/parallel/test-http2-invalid-last-stream-id.js

@@ -0,0 +1,77 @@
+// Flags: --expose-internals
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto) common.skip('missing crypto');
+
+const h2 = require('http2');
+const net = require('net');
+const assert = require('assert');
+const { ServerHttp2Session } = require('internal/http2/core');
+
+async function sendInvalidLastStreamId(server) {
+  const client = new net.Socket();
+
+  const address = server.address();
+  if (!common.hasIPv6 && address.family === 'IPv6') {
+    // Necessary to pass CI running inside containers.
+    client.connect(address.port);
+  } else {
+    client.connect(address);
+  }
+
+  client.on('connect', common.mustCall(function() {
+    // HTTP/2 preface
+    client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8'));
+
+    // Empty SETTINGS frame
+    client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]));
+
+    // GOAWAY frame with custom debug message
+    const goAwayFrame = [
+      0x00, 0x00, 0x21, // Length: 33 bytes
+      0x07, // Type: GOAWAY
+      0x00, // Flags
+      0x00, 0x00, 0x00, 0x00, // Stream ID: 0
+      0x00, 0x00, 0x00, 0x01, // Last Stream ID: 1
+      0x00, 0x00, 0x00, 0x00, // Error Code: 0 (No error)
+    ];
+
+    // Add the debug message
+    const debugMessage = 'client transport shutdown';
+    const goAwayBuffer = Buffer.concat([
+      Buffer.from(goAwayFrame),
+      Buffer.from(debugMessage, 'utf8'),
+    ]);
+
+    client.write(goAwayBuffer);
+    client.destroy();
+  }));
+}
+
+const server = h2.createServer();
+
+server.on('error', common.mustNotCall());
+
+server.on(
+  'sessionError',
+  common.mustCall((err, session) => {
+    // When destroying the session, on Windows, we would get ECONNRESET
+    // errors, make sure we take those into account in our tests.
+    if (err.code !== 'ECONNRESET') {
+      assert.strictEqual(err.code, 'ERR_HTTP2_ERROR');
+      assert.strictEqual(err.name, 'Error');
+      assert.strictEqual(err.message, 'Protocol error');
+      assert.strictEqual(session instanceof ServerHttp2Session, true);
+    }
+    session.close();
+    server.close();
+  }),
+);
+
+server.listen(
+  0,
+  common.mustCall(async () => {
+    await sendInvalidLastStreamId(server);
+  }),
+);

test/parallel/test-http2-options-max-headers-block-length.js

@@ -35,9 +35,11 @@ server.listen(0, common.mustCall(() => {
     assert.strictEqual(code, h2.constants.NGHTTP2_FRAME_SIZE_ERROR);
   }));
 
+  // NGHTTP2 will automatically send the NGHTTP2_REFUSED_STREAM with
+  // the GOAWAY frame.
   req.on('error', common.expectsError({
     code: 'ERR_HTTP2_STREAM_ERROR',
     name: 'Error',
-    message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR'
+    message: 'Stream closed with error code NGHTTP2_REFUSED_STREAM'
   }));
 }));

test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js

@@ -59,6 +59,9 @@ server.listen(0, common.mustCall(() => {
     'session',
     common.mustCall((session) => {
       assert.strictEqual(session instanceof ServerHttp2Session, true);
+      session.on('close', common.mustCall(() => {
+        server.close();
+      }));
     }),
   );
   server.on(
@@ -80,7 +83,6 @@ server.listen(0, common.mustCall(() => {
       assert.strictEqual(err.name, 'Error');
       assert.strictEqual(err.message, 'Session closed with error code 9');
       assert.strictEqual(session instanceof ServerHttp2Session, true);
-      server.close();
     }),
   );
 

test/parallel/test-http2-premature-close.js

@@ -0,0 +1,88 @@
+// Flags: --expose-internals
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto) common.skip('missing crypto');
+
+const h2 = require('http2');
+const net = require('net');
+
+async function requestAndClose(server) {
+  const client = new net.Socket();
+
+  const address = server.address();
+  if (!common.hasIPv6 && address.family === 'IPv6') {
+    // Necessary to pass CI running inside containers.
+    client.connect(address.port);
+  } else {
+    client.connect(address);
+  }
+
+  client.on('connect', common.mustCall(function() {
+    // Send HTTP/2 Preface
+    client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8'));
+
+    // Send a SETTINGS frame (empty payload)
+    client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]));
+
+    const streamId = 1;
+    // Send a valid HEADERS frame
+    const headersFrame = Buffer.concat([
+      Buffer.from([
+        0x00, 0x00, 0x0c, // Length: 12 bytes
+        0x01, // Type: HEADERS
+        0x05, // Flags: END_HEADERS + END_STREAM
+        (streamId >> 24) & 0xFF, // Stream ID: high byte
+        (streamId >> 16) & 0xFF,
+        (streamId >> 8) & 0xFF,
+        streamId & 0xFF, // Stream ID: low byte
+      ]),
+      Buffer.from([
+        0x82, // Indexed Header Field Representation (Predefined ":method: GET")
+        0x84, // Indexed Header Field Representation (Predefined ":path: /")
+        0x86, // Indexed Header Field Representation (Predefined ":scheme: http")
+        0x44, 0x0a, // Custom ":authority: localhost"
+        0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74,
+      ]),
+    ]);
+    client.write(headersFrame);
+
+    // Send a valid DATA frame
+    const dataFrame = Buffer.concat([
+      Buffer.from([
+        0x00, 0x00, 0x05, // Length: 5 bytes
+        0x00, // Type: DATA
+        0x00, // Flags: No flags
+        (streamId >> 24) & 0xFF, // Stream ID: high byte
+        (streamId >> 16) & 0xFF,
+        (streamId >> 8) & 0xFF,
+        streamId & 0xFF, // Stream ID: low byte
+      ]),
+      Buffer.from('Hello', 'utf8'), // Data payload
+    ]);
+    client.write(dataFrame);
+
+    // Does not wait for server to reply. Shutdown the socket
+    client.end();
+  }));
+}
+
+const server = h2.createServer();
+
+server.on('error', common.mustNotCall());
+
+server.on(
+  'session',
+  common.mustCall((session) => {
+    session.on('close', common.mustCall(() => {
+      server.close();
+    }));
+  }),
+);
+
+server.listen(
+  0,
+  common.mustCall(async () => {
+    await requestAndClose(server);
+  }),
+);