http2: fix memory leak on nghttp2 hd threshold

RafaelGSS · danielleadams · commit 7bf2be51b3ac · 2022-03-14T09:32:08.000-04:00
PR-URL: #41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -1009,6 +1009,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
   return 0;
 }
 
+// Remove the headers reference.
+// Implicitly calls nghttp2_rcbuf_decref
+void Http2Session::DecrefHeaders(const nghttp2_frame* frame) {
+  int32_t id = GetFrameID(frame);
+  BaseObjectPtr<Http2Stream> stream = FindStream(id);
+
+  if (stream && !stream->is_destroyed() && stream->headers_count() > 0) {
+    Debug(this, "freeing headers for stream %d", id);
+    stream->ClearHeaders();
+    CHECK_EQ(stream->headers_count(), 0);
+    DecrementCurrentSessionMemory(stream->current_headers_length_);
+    stream->current_headers_length_ = 0;
+  }
+}
+
 // If nghttp2 is unable to send a queued up frame, it will call this callback
 // to let us know. If the failure occurred because we are in the process of
 // closing down the session or stream, we go ahead and ignore it. We don't
@@ -1029,6 +1044,11 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
       error_code == NGHTTP2_ERR_STREAM_CLOSED ||
       error_code == NGHTTP2_ERR_STREAM_CLOSING ||
       session->js_fields_->frame_error_listener_count == 0) {
+    // Nghttp2 contains header limit of 65536. When this value is exceeded the
+    // pipeline is stopped and we should remove the current headers reference
+    // to destroy the session completely.
+    // Further information see: https://github.com/nodejs/node/issues/35233
+    session->DecrefHeaders(frame);
     return 0;
   }
 
diff --git a/src/node_http2.h b/src/node_http2.h
@@ -401,6 +401,10 @@ class Http2Stream : public AsyncWrap,
     size_t i = 0;
     for (const auto& header : current_headers_ )
       fn(header, i++);
+    ClearHeaders();
+  }
+
+  void ClearHeaders() {
     current_headers_.clear();
   }
 
@@ -787,6 +791,8 @@ class Http2Session : public AsyncWrap,
   void HandleAltSvcFrame(const nghttp2_frame* frame);
   void HandleOriginFrame(const nghttp2_frame* frame);
 
+  void DecrefHeaders(const nghttp2_frame* frame);
+
   // nghttp2 callbacks
   static int OnBeginHeadersCallback(
       nghttp2_session* session,
diff --git a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const h2 = require('http2');
+
+const server = h2.createServer();
+
+server.on('stream', common.mustNotCall());
+server.on('error', common.mustNotCall());
+
+server.listen(0, common.mustCall(() => {
+
+  // Setting the maxSendHeaderBlockLength > nghttp2 threshold
+  // cause a 'sessionError' and no memory leak when session destroy
+  const options = {
+    maxSendHeaderBlockLength: 100000
+  };
+
+  const client = h2.connect(`http://localhost:${server.address().port}`,
+                            options);
+  client.on('error', common.expectsError({
+    code: 'ERR_HTTP2_SESSION_ERROR',
+    name: 'Error',
+    message: 'Session closed with error code 9'
+  }));
+
+  const req = client.request({
+    // Greater than 65536 bytes
+    'test-header': 'A'.repeat(90000)
+  });
+  req.on('response', common.mustNotCall());
+
+  req.on('close', common.mustCall(() => {
+    client.close();
+    server.close();
+  }));
+
+  req.on('error', common.expectsError({
+    code: 'ERR_HTTP2_SESSION_ERROR',
+    name: 'Error',
+    message: 'Session closed with error code 9'
+  }));
+  req.end();
+}));
