Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes

lib/_tls_wrap.js

@@ -704,6 +704,9 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromP
   defineHandleReading(this, handle);
 
   this.on('close', onSocketCloseDestroySSL);
+  if (wrap) {
+    wrap.on('close', () => this.destroy());
+  }
 
   return res;
 };

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

@@ -0,0 +1,67 @@
+'use strict';
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const net = require('net');
+const h2 = require('http2');
+
+const tlsOptions = {
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+  ALPNProtocols: ['h2']
+};
+
+// Create a net server that upgrades sockets to HTTP/2 manually, handles the
+// request, and then shuts down via a short socket timeout and a longer H2 session
+// timeout. This is an unconventional way to shut down a session (the underlying
+// socket closing first) but it should work - critically, it shouldn't segfault
+// (as it did until Node v20.5.1).
+
+let serverRawSocket;
+let serverH2Session;
+
+const netServer = net.createServer((socket) => {
+  serverRawSocket = socket;
+  h2Server.emit('connection', socket);
+});
+
+const h2Server = h2.createSecureServer(tlsOptions, (req, res) => {
+  res.writeHead(200);
+  res.end();
+});
+
+h2Server.on('session', (session) => {
+  serverH2Session = session;
+});
+
+netServer.listen(0, common.mustCall(() => {
+  const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
+    rejectUnauthorized: false
+  });
+
+  proxyClient.on('close', common.mustCall(() => {
+    netServer.close();
+  }));
+
+  const req = proxyClient.request({
+    ':method': 'GET',
+    ':path': '/'
+  });
+
+  req.on('response', common.mustCall((response) => {
+    assert.strictEqual(response[':status'], 200);
+
+    // Asynchronously shut down the server's connections after the response,
+    // but not in the order it typically expects:
+    setTimeout(() => {
+      serverRawSocket.destroy();
+
+      setTimeout(() => {
+        serverH2Session.close();
+      }, 10);
+    }, 10);
+  }));
+}));

test/parallel/test-tls-socket-close.js

@@ -8,73 +8,51 @@ const tls = require('tls');
 const net = require('net');
 const fixtures = require('../common/fixtures');
 
-// Regression test for https://github.com/nodejs/node/issues/8074
-//
-// This test has a dependency on the order in which the TCP connection is made,
-// and TLS server handshake completes. It assumes those server side events occur
-// before the client side write callback, which is not guaranteed by the TLS
-// API. It usually passes with TLS1.3, but TLS1.3 didn't exist at the time the
-// bug existed.
-//
-// Pin the test to TLS1.2, since the test shouldn't be changed in a way that
-// doesn't trigger a segfault in Node.js 7.7.3:
-//   https://github.com/nodejs/node/issues/13184#issuecomment-303700377
-tls.DEFAULT_MAX_VERSION = 'TLSv1.2';
-
 const key = fixtures.readKey('agent2-key.pem');
 const cert = fixtures.readKey('agent2-cert.pem');
 
-let tlsSocket;
-// tls server
+let serverTlsSocket;
 const tlsServer = tls.createServer({ cert, key }, (socket) => {
-  tlsSocket = socket;
-  socket.on('error', common.mustCall((error) => {
-    assert.strictEqual(error.code, 'EINVAL');
-    tlsServer.close();
-    netServer.close();
-  }));
+  serverTlsSocket = socket;
 });
 
+// A plain net server, that manually passes connections to the TLS
+// server to be upgraded
 let netSocket;
-// plain tcp server
 const netServer = net.createServer((socket) => {
-  // If client wants to use tls
   tlsServer.emit('connection', socket);
 
   netSocket = socket;
 }).listen(0, common.mustCall(function() {
   connectClient(netServer);
 }));
 
+// A client that connects, sends one message, and closes the raw connection:
 function connectClient(server) {
-  const tlsConnection = tls.connect({
+  const clientTlsSocket = tls.connect({
     host: 'localhost',
     port: server.address().port,
     rejectUnauthorized: false
   });
 
-  tlsConnection.write('foo', 'utf8', common.mustCall(() => {
+  clientTlsSocket.write('foo', 'utf8', common.mustCall(() => {
     assert(netSocket);
     netSocket.setTimeout(common.platformTimeout(10), common.mustCall(() => {
-      assert(tlsSocket);
-      // This breaks if TLSSocket is already managing the socket:
+      assert(serverTlsSocket);
+
       netSocket.destroy();
-      const interval = setInterval(() => {
-        // Checking this way allows us to do the write at a time that causes a
-        // segmentation fault (not always, but often) in Node.js 7.7.3 and
-        // earlier. If we instead, for example, wait on the `close` event, then
-        // it will not segmentation fault, which is what this test is all about.
-        if (tlsSocket._handle._parent.bytesRead === 0) {
-          tlsSocket.write('bar');
-          clearInterval(interval);
-        }
-      }, 1);
+
+      setImmediate(() => {
+        assert.strictEqual(netSocket.destroyed, true);
+        assert.strictEqual(clientTlsSocket.destroyed, true);
+
+        setImmediate(() => {
+          assert.strictEqual(serverTlsSocket.destroyed, true);
+
+          tlsServer.close();
+          netServer.close();
+        });
+      });
     }));
   }));
-  tlsConnection.on('error', (e) => {
-    // Tolerate the occasional ECONNRESET.
-    // Ref: https://github.com/nodejs/node/issues/13184
-    if (e.code !== 'ECONNRESET')
-      throw e;
-  });
 }