Skip to content

Commit

Permalink
tls: ensure TLS Sockets are closed if the underlying wrap closes
Browse files Browse the repository at this point in the history
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.

PR-URL: #49327
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
  • Loading branch information
pimterry authored and UlisesGascon committed Sep 10, 2023
1 parent 2a35782 commit 8949cc7
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 43 deletions.
3 changes: 3 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
67 changes: 67 additions & 0 deletions test/parallel/test-http2-socket-close.js
Original file line number Diff line number Diff line change
@@ -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);
}));
}));
64 changes: 21 additions & 43 deletions test/parallel/test-tls-socket-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
}

0 comments on commit 8949cc7

Please sign in to comment.