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

[pimterry]

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

I found this while investigating #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.

[aduh95]

parallel/test-http2-socket-close is failing on ASan and macOS CIs.

[pimterry]

parallel/test-http2-socket-close is failing on ASan and macOS CIs.

Thanks @aduh95! It seems this fails because the new segfault test throws a connection reset error on the client H2 stream.

That's not a huge problem: it's the server that segfaults without this change, so the test is really interested in server behaviour here, not the client, and a handleable connection error event is far better than a segfault anyway. That said, it's weird that it's different for Mac & ASAN compared to the other cases, I wonder if it's just a timing issue?

I've pushed a change to the tests, to see whether it's a timing problem, which moves the shutdown logic into timers started after the response is definitely fully received. Hopefully that will ensure the client session handles this more cleanly. The test still segfaults on Node 20.5.1 and passes reliably with this change (on my Linux machine).

If this test still doesn't pass in the same cases, I think just ignoring client errors for this case is fine - it's an unusual shutdown regardless, and really we just want to check the server doesn't crash, so I don't want to get distracted too much by independent client issues (even if they might be worth investigating elsewhere later).

[pimterry]

I've pushed a change to the tests, to see whether it's a timing problem, which moves the shutdown logic into timers started after the response is definitely fully received.

Seems it was indeed a timing issue - all tests are now passing everywhere 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53608/

[mcollina]

lgtm

[pimterry]

I can see two CI failures here:

• win-v2019-arm64 which fails due to a 60 minute timeout cross-compiling to ARM64
• node-test-commit-osx on osx 10.15 which fails with "write error: No space left on device"

Both seem very unlikely to be related to this change, so I'm going to assume this is all good and done, and those will presumably pass later on (maybe once that node's disk space is cleaned up?).

Thanks for the review @mcollina! I think this is ready to merge but let me know if there's anything else I should do to make that happen.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53650/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/49327
✔  Done loading data for nodejs/node/pull/49327
----------------------------------- PR info ------------------------------------
Title      Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes (#49327)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pimterry:fix-tls-shutdown -> nodejs:main
Labels     tls, author ready
Commits    2
 - tls: ensure TLS Sockets are closed if the underlying wrap closes
 - Tweak new H2 shutdown test towards reliable ordering & client shutdown
Committers 1
 - Tim Perry 
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Aug 2023 16:11:30 GMT
   ✔  Approvals: 1
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49327#pullrequestreview-1600235070
   ✘  This PR needs to wait 41 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-30T19:32:01Z: https://ci.nodejs.org/job/node-test-pull-request/53650/
- Querying data for job/node-test-pull-request/53650/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6030922076

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/49327
✔  Done loading data for nodejs/node/pull/49327
----------------------------------- PR info ------------------------------------
Title      Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes (#49327)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pimterry:fix-tls-shutdown -> nodejs:main
Labels     tls, author ready, commit-queue-squash
Commits    2
 - tls: ensure TLS Sockets are closed if the underlying wrap closes
 - Tweak new H2 shutdown test towards reliable ordering & client shutdown
Committers 1
 - Tim Perry 
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Aug 2023 16:11:30 GMT
   ✔  Approvals: 1
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49327#pullrequestreview-1600235070
   ✘  This PR needs to wait 27 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-30T23:03:30Z: https://ci.nodejs.org/job/node-test-pull-request/53650/
- Querying data for job/node-test-pull-request/53650/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6037572193

[nodejs-github-bot]

Landed in 048e0be

[lpinca]

@pimterry the parallel/test-tls-socket-close.js test is flaky now. An assertion fails

=== release test-tls-socket-close ===                   
Path: parallel/test-tls-socket-close
node:assert:125
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at Immediate._onImmediate (/Users/luigi/code/node/test/parallel/test-tls-socket-close.js:47:16)
    at process.processImmediate (node:internal/timers:478:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}

Node.js v21.0.0-pre
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-tls-socket-close.js

I can reproduce the issue on macOS by running

python3 tools/test.py -J --repeat=1000 test/parallel/test-tls-socket-close.js

[lpinca]

The following patch seems to fix the issue

diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js
index 667b291309..70af760d53 100644
--- a/test/parallel/test-tls-socket-close.js
+++ b/test/parallel/test-tls-socket-close.js
@@ -44,9 +44,9 @@ function connectClient(server) {
 
       setImmediate(() => {
         assert.strictEqual(netSocket.destroyed, true);
-        assert.strictEqual(clientTlsSocket.destroyed, true);
 
         setImmediate(() => {
+          assert.strictEqual(clientTlsSocket.destroyed, true);
           assert.strictEqual(serverTlsSocket.destroyed, true);
 
           tlsServer.close();

but I'm not sure if it invalidates some assumptions.

[pimterry]

Ah, sorry about that. I think that patch absolutely makes sense - there's no specific reason that it should be exactly one setImmediate to get to the destroyed state on the remote socket, it just needs to settle there eventually.

I assume you're happy to PR that patch, but let me know if you'd prefer me to do it.

[lpinca]

Done, thanks for the fast reply.