diff --git a/lib/internal/js_stream_socket.js b/lib/internal/js_stream_socket.js index 8bc19296620b3f..68e1802a63b012 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 00000000000000..6e04121ca71ea8 --- /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 + })); +}));