chore: cherry-pick tls shutdown crash fix from upstream (#39947)

* chore: cherry-pick tls shutdown crash fix from upstream

Co-authored-by: deepak1556 <[email protected]>

* chore: update patches

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: deepak1556 <[email protected]>
Co-authored-by: John Kleinschmidt <[email protected]>

patches/node/.patches

@@ -42,3 +42,7 @@ fix_isurl_implementation.patch
 ci_ensure_node_tests_set_electron_run_as_node.patch
 chore_update_fixtures_errors_force_colors_snapshot.patch
 fix_assert_module_in_the_renderer_process.patch
+tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch
+test_deflake_test-tls-socket-close.patch
+net_fix_crash_due_to_simultaneous_close_shutdown_on_js_stream.patch
+net_use_asserts_in_js_socket_stream_to_catch_races_in_future.patch

patches/node/net_fix_crash_due_to_simultaneous_close_shutdown_on_js_stream.patch

@@ -0,0 +1,171 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tim Perry <[email protected]>
+Date: Thu, 24 Aug 2023 16:05:02 +0100
+Subject: net: fix crash due to simultaneous close/shutdown on JS Stream
+ Sockets
+
+A JS stream socket wraps a stream, exposing it as a socket for something
+on top which needs a socket specifically (e.g. an HTTP server).
+
+If the internal stream is closed in the same tick as the layer on top
+attempts to close this stream, the race between doShutdown and doClose
+results in an uncatchable exception. A similar race can happen with
+doClose and doWrite.
+
+It seems legitimate these can happen in parallel, so this resolves that
+by explicitly detecting and handling that situation: if a close is in
+progress, both doShutdown & doWrite allow doClose to run
+finishShutdown/Write for them, cancelling the operation, without trying
+to use this._handle (which will be null) in the meantime.
+
+PR-URL: https://github.com/nodejs/node/pull/49400
+Reviewed-By: Matteo Collina <[email protected]>
+Reviewed-By: Luigi Pinca <[email protected]>
+
+diff --git a/lib/internal/js_stream_socket.js b/lib/internal/js_stream_socket.js
+index 8bc19296620b3fd0e5487165743f0f1bc2d342e7..68e1802a63b012b59418b79a0e68de5147543a23 100644
+--- a/lib/internal/js_stream_socket.js
++++ b/lib/internal/js_stream_socket.js
+@@ -21,6 +21,7 @@ const { ERR_STREAM_WRAP } = require('internal/errors').codes;
+ const kCurrentWriteRequest = Symbol('kCurrentWriteRequest');
+ const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest');
+ const kPendingShutdownRequest = Symbol('kPendingShutdownRequest');
++const kPendingClose = Symbol('kPendingClose');
+ 
+ function isClosing() { return this[owner_symbol].isClosing(); }
+ 
+@@ -94,6 +95,7 @@ class JSStreamSocket extends Socket {
+     this[kCurrentWriteRequest] = null;
+     this[kCurrentShutdownRequest] = null;
+     this[kPendingShutdownRequest] = null;
++    this[kPendingClose] = false;
+     this.readable = stream.readable;
+     this.writable = stream.writable;
+ 
+@@ -135,10 +137,17 @@ class JSStreamSocket extends Socket {
+       this[kPendingShutdownRequest] = req;
+       return 0;
+     }
++
+     assert(this[kCurrentWriteRequest] === null);
+     assert(this[kCurrentShutdownRequest] === null);
+     this[kCurrentShutdownRequest] = req;
+ 
++    if (this[kPendingClose]) {
++      // If doClose is pending, the stream & this._handle are gone. We can't do
++      // anything. doClose will call finishShutdown with ECANCELED for us shortly.
++      return 0;
++    }
++
+     const handle = this._handle;
+ 
+     process.nextTick(() => {
+@@ -164,6 +173,13 @@ class JSStreamSocket extends Socket {
+     assert(this[kCurrentWriteRequest] === null);
+     assert(this[kCurrentShutdownRequest] === null);
+ 
++    if (this[kPendingClose]) {
++      // If doClose is pending, the stream & this._handle are gone. We can't do
++      // anything. doClose will call finishWrite with ECANCELED for us shortly.
++      this[kCurrentWriteRequest] = req; // Store req, for doClose to cancel
++      return 0;
++    }
++
+     const handle = this._handle;
+     const self = this;
+ 
+@@ -217,6 +233,8 @@ class JSStreamSocket extends Socket {
+   }
+ 
+   doClose(cb) {
++    this[kPendingClose] = true;
++
+     const handle = this._handle;
+ 
+     // When sockets of the "net" module destroyed, they will call
+@@ -234,6 +252,8 @@ class JSStreamSocket extends Socket {
+       this.finishWrite(handle, uv.UV_ECANCELED);
+       this.finishShutdown(handle, uv.UV_ECANCELED);
+ 
++      this[kPendingClose] = false;
++
+       cb();
+     });
+   }
+diff --git a/test/parallel/test-http2-client-connection-tunnelling.js b/test/parallel/test-http2-client-connection-tunnelling.js
+new file mode 100644
+index 0000000000000000000000000000000000000000..6e04121ca71ea81f49c7f50ec11d7fac735c80a9
+--- /dev/null
++++ b/test/parallel/test-http2-client-connection-tunnelling.js
+@@ -0,0 +1,71 @@
++'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');
++
++// This test sets up an H2 proxy server, and tunnels a request over one of its streams
++// back to itself, via TLS, and then closes the TLS connection. On some Node versions
++// (v18 & v20 up to 20.5.1) the resulting JS Stream Socket fails to shutdown correctly
++// in this case, and crashes due to a null pointer in finishShutdown.
++
++const tlsOptions = {
++  key: fixtures.readKey('agent1-key.pem'),
++  cert: fixtures.readKey('agent1-cert.pem'),
++  ALPNProtocols: ['h2']
++};
++
++const netServer = net.createServer((socket) => {
++  socket.allowHalfOpen = false;
++  // ^ This allows us to trigger this reliably, but it's not strictly required
++  // for the bug and crash to happen, skipping this just fails elsewhere later.
++
++  h2Server.emit('connection', socket);
++});
++
++const h2Server = h2.createSecureServer(tlsOptions, (req, res) => {
++  res.writeHead(200);
++  res.end();
++});
++
++h2Server.on('connect', (req, res) => {
++  res.writeHead(200, {});
++  netServer.emit('connection', res.stream);
++});
++
++netServer.listen(0, common.mustCall(() => {
++  const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
++    rejectUnauthorized: false
++  });
++
++  const proxyReq = proxyClient.request({
++    ':method': 'CONNECT',
++    ':authority': 'example.com:443'
++  });
++
++  proxyReq.on('response', common.mustCall((response) => {
++    assert.strictEqual(response[':status'], 200);
++
++    // Create a TLS socket within the tunnel, and start sending a request:
++    const tlsSocket = tls.connect({
++      socket: proxyReq,
++      ALPNProtocols: ['h2'],
++      rejectUnauthorized: false
++    });
++
++    proxyReq.on('close', common.mustCall(() => {
++      proxyClient.close();
++      netServer.close();
++    }));
++
++    // Forcibly kill the TLS socket
++    tlsSocket.destroy();
++
++    // This results in an async error in affected Node versions, before the 'close' event
++  }));
++}));

patches/node/net_use_asserts_in_js_socket_stream_to_catch_races_in_future.patch

@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tim Perry <[email protected]>
+Date: Fri, 25 Aug 2023 14:16:35 +0100
+Subject: net: use asserts in JS Socket Stream to catch races in future
+
+PR-URL: https://github.com/nodejs/node/pull/49400
+Reviewed-By: Matteo Collina <[email protected]>
+Reviewed-By: Luigi Pinca <[email protected]>
+
+diff --git a/lib/internal/js_stream_socket.js b/lib/internal/js_stream_socket.js
+index 68e1802a63b012b59418b79a0e68de5147543a23..70d6d03069f3f1e85e66864c6c1e6de6084f5ea6 100644
+--- a/lib/internal/js_stream_socket.js
++++ b/lib/internal/js_stream_socket.js
+@@ -149,6 +149,7 @@ class JSStreamSocket extends Socket {
+     }
+ 
+     const handle = this._handle;
++    assert(handle !== null);
+ 
+     process.nextTick(() => {
+       // Ensure that write is dispatched asynchronously.
+@@ -181,6 +182,8 @@ class JSStreamSocket extends Socket {
+     }
+ 
+     const handle = this._handle;
++    assert(handle !== null);
++
+     const self = this;
+ 
+     let pending = bufs.length;

patches/node/test_deflake_test-tls-socket-close.patch

@@ -0,0 +1,28 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Luigi Pinca <[email protected]>
+Date: Wed, 13 Sep 2023 08:04:39 +0200
+Subject: test: deflake test-tls-socket-close
+
+Move the check for the destroyed state of the remote socket to the inner
+`setImmediate()`.
+
+Refs: https://github.com/nodejs/node/pull/49327#issuecomment-1712525257
+PR-URL: https://github.com/nodejs/node/pull/49575
+Reviewed-By: Joyee Cheung <[email protected]>
+Reviewed-By: Moshe Atlow <[email protected]>
+
+diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js
+index 667b291309a4c5636a2c658fa8204b32c2e4df46..70af760d53bb4ddab62c99180d505e943ec269f6 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();

patches/node/tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch

@@ -0,0 +1,204 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tim Perry <[email protected]>
+Date: Fri, 1 Sep 2023 09:00:05 +0200
+Subject: 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.
+
+PR-URL: https://github.com/nodejs/node/pull/49327
+Reviewed-By: Matteo Collina <[email protected]>
+Reviewed-By: Debadree Chatterjee <[email protected]>
+
+diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
+index 1eff3b3fba8a05d5fade43c6cb00b6621daa8c3d..5bed23e1355e5e5a1965f15ca270fb372f751d66 100644
+--- a/lib/_tls_wrap.js
++++ b/lib/_tls_wrap.js
+@@ -629,6 +629,9 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle) {
+   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 0000000000000000000000000000000000000000..02db77bcf8480c79c77175ba802f9fe10ffc4efe
+--- /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 87355cf8d7bd2d07bb0fab59491b68f3963f8809..667b291309a4c5636a2c658fa8204b32c2e4df46 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;
+-  });
+ }