http2: fix memory leak caused by premature listener removing

ywave620 · marco-ippolito · commit 7ba6dcf180a7 · 2025-01-24T22:18:46.000+01:00
Http2Session should always call ondone into JS to detach the handle. In some case, ondone is defered to be called by the StreamListener through WriteWrap, we should be careful of this before getting rid of the StreamListener. PR-URL: #55966 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -792,13 +792,15 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
     CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0);
     SendPendingData();
   } else if (stream_ != nullptr) {
+    // so that the previous listener of the socket, typically, JS code of a
+    // (tls) socket will be notified of any activity later
     stream_->RemoveStreamListener(this);
   }
 
   set_destroyed();
 
   // If we are writing we will get to make the callback in OnStreamAfterWrite.
-  if (!is_write_in_progress()) {
+  if (!is_write_in_progress() || !stream_) {
     Debug(this, "make done session callback");
     HandleScope scope(env()->isolate());
     MakeCallback(env()->ondone_string(), 0, nullptr);
diff --git a/test/parallel/test-h2leak-destroy-session-on-socket-ended.js b/test/parallel/test-h2leak-destroy-session-on-socket-ended.js
@@ -0,0 +1,80 @@
+'use strict';
+// Flags: --expose-gc
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+
+const registry = new FinalizationRegistry(common.mustCall((name) => {
+  assert(name, 'session');
+}));
+
+const server = http2.createSecureServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+});
+
+let firstServerStream;
+
+
+server.on('secureConnection', (s) => {
+  console.log('secureConnection');
+  s.on('end', () => {
+    console.log(s.destroyed); // false !!
+    s.destroy();
+    firstServerStream.session.destroy();
+
+    firstServerStream = null;
+
+    setImmediate(() => {
+      global.gc();
+      global.gc();
+
+      server.close();
+    });
+  });
+});
+
+server.on('session', (s) => {
+  registry.register(s, 'session');
+});
+
+server.on('stream', (stream) => {
+  console.log('stream...');
+  stream.write('a'.repeat(1024));
+  firstServerStream = stream;
+  setImmediate(() => console.log('Draining setImmediate after writing'));
+});
+
+
+server.listen(() => {
+  client();
+});
+
+
+const h2fstStream = [
+  'UFJJICogSFRUUC8yLjANCg0KU00NCg0K',
+  // http message (1st stream:)
+  'AAAABAAAAAAA',
+  'AAAPAQUAAAABhIJBiqDkHROdCbjwHgeG',
+];
+function client() {
+  const client = tls.connect({
+    port: server.address().port,
+    host: 'localhost',
+    rejectUnauthorized: false,
+    ALPNProtocols: ['h2']
+  }, () => {
+    client.end(Buffer.concat(h2fstStream.map((s) => Buffer.from(s, 'base64'))), (err) => {
+      assert.ifError(err);
+    });
+  });
+
+  client.on('error', (error) => {
+    console.error('Connection error:', error);
+  });
+}
