forked from electron/electron
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick tls shutdown crash fix from upstream (electron#39928)
- Loading branch information
1 parent
ba89152
commit 5f712fa
Showing
5 changed files
with
437 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
171 changes: 171 additions & 0 deletions
171
patches/node/net_fix_crash_due_to_simultaneous_close_shutdown_on_js_stream.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
+ })); | ||
+})); |
30 changes: 30 additions & 0 deletions
30
patches/node/net_use_asserts_in_js_socket_stream_to_catch_races_in_future.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); |
Oops, something went wrong.