diff --git a/patches/node/.patches b/patches/node/.patches index 774b79f5303f3..988022f078811 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -44,3 +44,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 diff --git a/patches/node/net_fix_crash_due_to_simultaneous_close_shutdown_on_js_stream.patch b/patches/node/net_fix_crash_due_to_simultaneous_close_shutdown_on_js_stream.patch new file mode 100644 index 0000000000000..4f1bc6eb5a608 --- /dev/null +++ b/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 +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 +Reviewed-By: Luigi Pinca + +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 ++ })); ++})); diff --git a/patches/node/net_use_asserts_in_js_socket_stream_to_catch_races_in_future.patch b/patches/node/net_use_asserts_in_js_socket_stream_to_catch_races_in_future.patch new file mode 100644 index 0000000000000..ac99478f3278f --- /dev/null +++ b/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 +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 +Reviewed-By: Luigi Pinca + +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; diff --git a/patches/node/test_deflake_test-tls-socket-close.patch b/patches/node/test_deflake_test-tls-socket-close.patch new file mode 100644 index 0000000000000..5ef091eb03494 --- /dev/null +++ b/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 +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 +Reviewed-By: Moshe Atlow + +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(); diff --git a/patches/node/tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch b/patches/node/tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch new file mode 100644 index 0000000000000..4761fd287c12b --- /dev/null +++ b/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 <1526883+pimterry@users.noreply.github.com> +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 +Reviewed-By: Debadree Chatterjee + +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; +- }); + }