electron · jkleinsc · Sep 21, 2023 · Sep 20, 2023
@@ -45,3 +45,7 @@ chore_update_fixtures_errors_force_colors_snapshot.patch
 fix_assert_module_in_the_renderer_process.patch
 src_cast_v8_object_getinternalfield_return_value_to_v8_value.patch
 fix_add_trusted_space_and_trusted_lo_space_to_the_v8_heap.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
@@ -0,0 +1,171 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tim Perry <pimterry@gmail.com>
+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 <matteo.collina@gmail.com>
+Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
+
+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
++  }));
++}));
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tim Perry <pimterry@gmail.com>
+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 <matteo.collina@gmail.com>
+Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
+
+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;
@@ -0,0 +1,28 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Luigi Pinca <luigipinca@gmail.com>
+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 <joyeec9h3@gmail.com>
+Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
+
+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();