Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/pimterry/fix-tls-shutdown into main

This fixes a potential HTTP/2 segfault (https://github.com/nodejs/node/issues/49307, https://github.com/nodejs/node/issues/48567) caused by underlying TLS behaviour, and likely fixes various other apparently related issues (https://github.com/nodejs/node/issues/46094, https://github.com/nodejs/node/issues/35695).

I found this while investigating https://github.com/nodejs/node/issues/48519, which this doesn't directly fix, but definitely relates to, as the JS Stream Sockets there have all sorts of issues directly caused by TLS interactions after the JSS is closed.

The underlying problem here is that TLS sockets which are created by passing an existing socket (either tlsServer.emit('connection', socket) or tls.createConnection({ ..., socket })) do not listen to whether the underlying socket was closed elsewhere. This is easy to demo:

const tlsOptions = {
  key: // Any key, e.g. fixtures.readKey('agent1-key.pem'),
  cert: // Any cert, e.g. fixtures.readKey('agent1-cert.pem')
};

const net = require('net');
const tls = require('tls');

const server = tls.createServer(tlsOptions, () => {
    console.log('Got TLS connection');
});

server.listen(0, () => {
    const socket = net.createConnection({
        port: server.address().port
    });

    socket.on('connect', () => {
        const tlsSocket = tls.connect({
            socket,
            rejectUnauthorized: false
        });

        tlsSocket.on('secureConnect', () => {
            console.log('TLS client connected');

            setImmediate(() => {
                socket.destroy();

                setTimeout(() => {
                    console.log('Is the socket alive?', !socket.destroyed);
                    console.log('Is TLS readable & writable?', tlsSocket.writable, tlsSocket.readable);
                }, 100);
            });
        });
    });
});

This prints:

TLS client connected
Got TLS connection
Is the socket alive? false
Is TLS readable & writable? true true

With this change, the last line is instead false false.

Clearly the TLS socket should not be readable or writeable 100ms after it's definitely disconnected. This happens even if something is reading data - the data just stops, and the affected TLS socket (the client in this case) never closes.

These TLSSockets do seem to shut down correctly if the remote side closes the connection, but in any other scenario, the TLS connection will remain happily active like this even when the underlying socket is gone, until something more serious interacts with its internals, which then explode (either segfaults in normal cases, or the various finishWrite errors in the issues above in JS Stream Socket cases).

The first segfault is definitely caused by this case, and I've added a new test for that.

I'm fairly confident this is the underlying issue for the other cases too - note that both cases use the manual socket handling pattern (with HTTP/2, but implicitly on top of TLS) - but neither has a reliable repro to confirm that.

This change notably significantly modifies an existing TLS test too. Unfortunately with this change in place, that test no longer works at all, as it tries to mess with internals to trigger a race condition that crashed in some Node v7 versions, and those internals are now cleaned up before they can be messed with. This test has been simplified to a more general TLS shutdown test instead.

There is some history to this - very similar code did used to exist!

  • It was added here
  • It was then reverted here on the basis (discussed here) that it was now unnecessary, due to other changes.
  • That caused issues, which were fixed for the StreamWrap case only here
  • That was reverted as an unnecessary special case here

@addaleax and @ronag will likely be interested in this, since they were involved at various stages of that.

I think there's a good case here that something explicitly handling this is necessary. Somehow, TLS sockets need to be informed if the underlying socket is closed. Note that this change as here is not specific to StreamWraps, unless some past changes. AFAICT this is required here for all manually constructed TLS cases - the new test here just passes a net.Socket directly and it still crashes on all current Node versions.

Merge request reports