diff --git a/patches/node/.patches b/patches/node/.patches index 11ee2997aec57..afa2598d8dbf0 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -36,7 +36,6 @@ fix_ftbfs_werror_wextra-semi.patch ci_ensure_node_tests_set_electron_run_as_node.patch fix_assert_module_in_the_renderer_process.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/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 deleted file mode 100644 index bb7c94f0cc8eb..0000000000000 --- a/patches/node/tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch +++ /dev/null @@ -1,204 +0,0 @@ -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 84599102be4b74ff66af63c36c232f4656a1f406..bb0535558a8999034a99d0e7e99507ba10eca2eb 100644 ---- a/lib/_tls_wrap.js -+++ b/lib/_tls_wrap.js -@@ -661,6 +661,9 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromP - 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; -- }); - }