Fix JS Stream Socket finishShutdown crash

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,11 +137,19 @@ 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;
+    assert(handle !== null);
 
     process.nextTick(() => {
       // Ensure that write is dispatched asynchronously.
@@ -164,7 +174,16 @@ 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;
+    assert(handle !== null);
+
     const self = this;
 
     let pending = bufs.length;
@@ -217,6 +236,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 +255,8 @@ class JSStreamSocket extends Socket {
       this.finishWrite(handle, uv.UV_ECANCELED);
       this.finishShutdown(handle, uv.UV_ECANCELED);
 
+      this[kPendingClose] = false;
+
       cb();
     });
   }

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
+  }));
+}));