From e068064c48bfd2dee25b6451e431e3ead546189c Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Fri, 25 Aug 2023 14:15:10 +0100 Subject: [PATCH] tls: ensure TLS Sockets are closed if the underlying wrap closes This fixes a potential segfault, among various other likely-related issues, which all occur because TLSSockets were not informed if their underlying stream was closed in many cases. This also significantly modifies an existing TLS test. With this change in place, that test no longer works, as it tries to mess with internals to trigger a race, and those internals are now cleaned up earlier. This test has been simplified to a more general TLS shutdown test. --- lib/_tls_wrap.js | 3 ++ test/parallel/test-http2-socket-close.js | 59 ++++++++++++++++++++++ test/parallel/test-tls-socket-close.js | 64 ++++++++---------------- 3 files changed, 83 insertions(+), 43 deletions(-) create mode 100644 test/parallel/test-http2-socket-close.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 909f36dd00fe155..24b9472da6b2d4e 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 000000000000000..fb197d27085f306 --- /dev/null +++ b/test/parallel/test-http2-socket-close.js @@ -0,0 +1,59 @@ +'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 tls = require('tls'); +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, but with two +// different shutdown timeouts: a short socket timeout, and a longer H2 session timeout. +// Since the only request is complete, the session should shutdown cleanly when the +// socket shuts down (and should _not_ segfault, as it does in Node v20.5.1) + +const netServer = net.createServer((socket) => { + setTimeout(() => { + socket.destroy(); + }, 10); + + h2Server.emit('connection', socket); +}); + +const h2Server = h2.createSecureServer(tlsOptions, (req, res) => { + res.writeHead(200); + res.end(); +}); + +h2Server.on('session', session => { + setTimeout(() => { + session.close(); + }, 20); +}); + +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.equal(response[':status'], 200); + }))); +})); diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js index 87355cf8d7bd2d0..667b291309a4c56 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; - }); }