diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 909f36dd00fe15..24b9472da6b2d4 100644 --- a/lib/_tls_wrap.js +++ b/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; }; diff --git a/test/parallel/test-http2-socket-close.js b/test/parallel/test-http2-socket-close.js new file mode 100644 index 00000000000000..02db77bcf8480c --- /dev/null +++ b/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); + })); +})); diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js index 87355cf8d7bd2d..667b291309a4c5 100644 --- a/test/parallel/test-tls-socket-close.js +++ b/test/parallel/test-tls-socket-close.js @@ -8,37 +8,18 @@ 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; @@ -46,35 +27,32 @@ const netServer = net.createServer((socket) => { 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; - }); }