node:tls: destroy the wrapped net.Socket when a TLS wrap is destroyed

src/js/node/net.ts

@@ -142,7 +142,6 @@ const kSyncWriteFd = Symbol("kSyncWriteFd");
 const kSetKeepAliveInitialDelay = Symbol("kSetKeepAliveInitialDelay");
 const kConnectOptions = Symbol("connect-options");
 const kAttach = Symbol("kAttach");
-const kCloseRawConnection = Symbol("kCloseRawConnection");
 const kupgraded = Symbol("kupgraded");
 const kAdoptedTLSRaw = Symbol("kAdoptedTLSRaw");
 const ksocket = Symbol("ksocket");
@@ -249,6 +248,23 @@ function closeAdoptedTLSRawNowNT(handle, self, isException) {
   handle.close(onSocketHandleClosed);
   setImmediate(emitCloseNT, self, isException);
 }
+// A TLS socket takes the transport it was layered over down with it, like
+// node's TLSWrap does its parent wrap: on the tick after _destroy, so the TLS
+// socket's 'error' still sees the transport intact and the transport's 'close'
+// precedes the TLS socket's. The raw twin of an adopted fd is closed by the
+// TLS handle, so it is detached rather than closed a second time.
+function destroyUpgradedTransportNT(upgraded) {
+  if (upgraded.destroyed) return;
+  if (upgraded._handle?.[kAdoptedTLSRaw]) {
+    upgraded._handle = null;
+  }
+  upgraded.destroy();
+}
+// Handle-less TLS socket: its 'close' is queued behind the transport's.
+function destroyUpgradedTransportAndCloseNT(upgraded, self, hasError) {
+  destroyUpgradedTransportNT(upgraded);
+  process.nextTick(emitCloseNT, self, hasError);
+}
 function detachSocket(self) {
   if (!self) self = this;
   self._handle = null;
@@ -1863,14 +1879,6 @@ Socket.prototype[kAttach] = function (port, socket) {
   SocketHandlers.drain(socket);
 };
 
-Socket.prototype[kCloseRawConnection] = function () {
-  const connection = this[kupgraded];
-  connection.connecting = false;
-  connection._handle = null;
-  connection.unref();
-  connection.destroy();
-};
-
 Socket.prototype.connect = function connect(...args) {
   $debug("Socket.prototype.connect");
   {
@@ -1989,8 +1997,10 @@ Socket.prototype.connect = function connect(...args) {
           // if is named pipe socket we can upgrade it using the same wrapper than we use for duplex
           upgradeDuplex = isNamedPipeSocket(socket) || hasUnflushedWrites(connection);
         }
+        // Before the upgrade, which may wait for 'connect': from here on
+        // destroying this socket destroys the connection too.
+        this[kupgraded] = connection;
         if (upgradeDuplex) {
-          this[kupgraded] = connection;
           const [result, events] = upgradeDuplexToTLS(connection, {
             data: { self: this, req: { oncomplete: afterConnect } },
             tls,
@@ -2006,7 +2016,6 @@ Socket.prototype.connect = function connect(...args) {
           // connecting (e.g. tls.connect({ socket: net.connect(port) })) must be
           // upgraded once it emits 'connect'.
           if (socket && !connection.connecting) {
-            this[kupgraded] = connection;
             const result = upgradeTLSDeferred(socket, {
               data: { self: this, req: { oncomplete: afterConnect } },
               tls,
@@ -2018,7 +2027,6 @@ Socket.prototype.connect = function connect(...args) {
               // replace socket
               connection._handle = raw;
               raw[kAdoptedTLSRaw] = true;
-              this.once("end", this[kCloseRawConnection]);
               raw.connecting = false;
               this._handle = tls;
             } else {
@@ -2028,20 +2036,14 @@ Socket.prototype.connect = function connect(...args) {
           } else {
             // wait to be connected
             connection.once("connect", () => {
-              // The TLS socket may have been destroyed before the underlying
-              // socket connected (e.g. tls.connect({ socket }).destroy()); don't
-              // start a handshake on a dead socket.
-              if (this.destroyed) {
-                connection.destroy();
-                return;
-              }
+              // Destroyed while connecting: _destroy is taking the connection down.
+              if (this.destroyed) return;
               const socket = connection._handle;
               if (!upgradeDuplex && socket) {
                 // if is named pipe socket we can upgrade it using the same wrapper than we use for duplex
                 upgradeDuplex = isNamedPipeSocket(socket) || hasUnflushedWrites(connection);
               }
               if (upgradeDuplex) {
-                this[kupgraded] = connection;
                 const [result, events] = upgradeDuplexToTLS(connection, {
                   data: { self: this, req: { oncomplete: afterConnect } },
                   tls,
@@ -2053,7 +2055,6 @@ Socket.prototype.connect = function connect(...args) {
                 connection.on("close", events[3]);
                 this._handle = result;
               } else {
-                this[kupgraded] = connection;
                 const result = upgradeTLSDeferred(socket, {
                   data: { self: this, req: { oncomplete: afterConnect } },
                   tls,
@@ -2065,7 +2066,6 @@ Socket.prototype.connect = function connect(...args) {
                   // replace socket
                   connection._handle = raw;
                   raw[kAdoptedTLSRaw] = true;
-                  this.once("end", this[kCloseRawConnection]);
                   raw.connecting = false;
                   this._handle = tls;
                 } else {
@@ -2141,14 +2141,7 @@ Socket.prototype._destroy = function _destroy(err, callback) {
   $debug("Socket.prototype._destroy");
 
   this.connecting = false;
-  // Tear down a wrapped generic duplex with this socket: the native handle's
-  // close only flushes close_notify and lets the wrapper drain; without an
-  // explicit destroy here a late RST on the underlying transport can surface
-  // as an unhandled error after this socket is gone.
   const upgraded = this[kupgraded];
-  if (upgraded && !(upgraded instanceof Socket) && !upgraded.destroyed) {
-    upgraded.destroy?.();
-  }
 
   // Close an fd adopted for synchronous writes (node closes the wrapping
   // libuv handle here). Leave stdio fds 0-2 open: process.stdout/stderr and
@@ -2214,9 +2207,15 @@ Socket.prototype._destroy = function _destroy(err, callback) {
       this._sockname = null;
     }
     callback(err);
+    // Queued after the 'error' that callback(err) queues.
+    if (upgraded) process.nextTick(destroyUpgradedTransportNT, upgraded);
   } else {
     callback(err);
-    process.nextTick(emitCloseNT, this, err ? true : false);
+    if (upgraded) {
+      process.nextTick(destroyUpgradedTransportAndCloseNT, upgraded, this, err ? true : false);
+    } else {
+      process.nextTick(emitCloseNT, this, err ? true : false);
+    }
   }
 
   const server = this._server;
@@ -2418,7 +2417,6 @@ Socket.prototype[Symbol.for("::bunUpgradeServerTLS::")] = function (connection,
     const [raw, tlsHandle] = result;
     connection._handle = raw;
     raw[kAdoptedTLSRaw] = true;
-    this.once("end", this[kCloseRawConnection]);
     raw.connecting = false;
     this._handle = tlsHandle;
     this.emit(kUpgradeAttached);

test/js/node/tls/node-tls-server.test.ts

@@ -1308,9 +1308,11 @@ it("SNICallback runs even when the requested servername matches the bind hostnam
   });
   server.listen(0, "localhost");
   await once(server, "listening");
-  const port = (server.address() as AddressInfo).port;
-  // host: "localhost" defaults servername to "localhost" - the bind hostname.
-  const client = connect({ port, host: "localhost", rejectUnauthorized: false });
+  const { port, address } = server.address() as AddressInfo;
+  // Connect to the literal bound address ("localhost" may resolve to a
+  // different family than the one the server picked) and request the bind
+  // hostname via SNI.
+  const client = connect({ port, host: address, servername: "localhost", rejectUnauthorized: false });
   await once(client, "secureConnect");
   expect(sniCalls).toBe(1);
   // The peer certificate must be the SNICallback's RSA cert, not COMMON_CERT.
@@ -2019,6 +2021,152 @@ it("destroys a server wrap whose socket was destroyed before the deferred upgrad
   }
 });
 
+// The converse of the test above: destroying the wrap has to take the wrapped
+// net.Socket down with it (node's TLSWrap close() destroys its parent wrap).
+// Otherwise the accepted socket never emits 'close', stays in the net.Server's
+// connection count and server.close(cb) never calls back. Releasing it from the
+// wrap's 'end' only covered a graceful shutdown: a handshake failure destroys
+// the wrap without ever emitting 'end'. Node's order of events, asserted below:
+// the wrap's 'error' / the server's 'tlsClientError' first, with the wrapped
+// socket still intact, then the wrapped socket's 'close', then the wrap's.
+describe("destroying a server-side TLSSocket wrap destroys the wrapped net.Socket", () => {
+  type Accepted = { raw: net.Socket; events: string[]; settled: Promise<void> };
+
+  // Logs both sockets' lifecycle events in order. `settled` resolves once the
+  // wrap's 'close' has been logged, plus the raw socket's if one is still due
+  // (a raw socket that was never destroyed, the bug, would never emit it, so
+  // that case settles immediately and fails on the log). Plain listeners rather
+  // than events.once(): the wrap emits 'error' before 'close' here.
+  function observe(raw: net.Socket, wrap: TLSSocket, events: string[] = []): Accepted {
+    const settled = Promise.withResolvers<void>();
+    raw.on("close", hadError => events.push(`raw close hadError=${hadError}`));
+    wrap.on("error", (err: Error & { code?: string }) =>
+      events.push(`wrap error ${err.code} raw.destroyed=${raw.destroyed}`),
+    );
+    wrap.on("close", hadError => {
+      events.push(`wrap close hadError=${hadError} raw.destroyed=${raw.destroyed}`);
+      if (raw.destroyed && !events.some(event => event.startsWith("raw close"))) raw.once("close", settled.resolve);
+      else settled.resolve();
+    });
+    return { raw, events, settled: settled.promise };
+  }
+
+  async function listenRaw(onConnection: (raw: net.Socket) => Accepted | Promise<Accepted>) {
+    const accepted = Promise.withResolvers<Accepted>();
+    const rawServer = net.createServer(raw => accepted.resolve(onConnection(raw)));
+    rawServer.on("error", accepted.reject);
+    rawServer.listen(0, "127.0.0.1");
+    await once(rawServer, "listening");
+    return { rawServer, port: (rawServer.address() as AddressInfo).port, accepted: accepted.promise };
+  }
+
+  async function expectReleased(rawServer: net.Server, { raw, events, settled }: Accepted, expected: string[]) {
+    await settled;
+    expect(events).toEqual(expected);
+    expect(raw.destroyed).toBe(true);
+    const connections = Promise.withResolvers<number>();
+    rawServer.getConnections((err, count) => (err ? connections.reject(err) : connections.resolve(count)));
+    expect(await connections.promise).toBe(0);
+    // With the connection released, close() completes.
+    const serverClosed = once(rawServer, "close");
+    rawServer.close();
+    await serverClosed;
+  }
+
+  it("when the handshake fails", async () => {
+    const { rawServer, port, accepted } = await listenRaw(raw =>
+      observe(raw, new TLSSocket(raw, { isServer: true, ...COMMON_CERT })),
+    );
+    // A plain TCP peer whose first bytes are not a ClientHello.
+    const client = net.connect(port, "127.0.0.1");
+    client.on("error", () => {});
+    try {
+      const conn = await accepted;
+      client.write("this is not a TLS ClientHello\r\n");
+      await expectReleased(rawServer, conn, [
+        "wrap error ERR_SSL_WRONG_VERSION_NUMBER raw.destroyed=false",
+        "raw close hadError=false",
+        "wrap close hadError=true raw.destroyed=true",
+      ]);
+    } finally {
+      client.destroy();
+      rawServer.close();
+    }
+  });
+
+  it("when a tls.Server rejects the client certificate of a connection handed to it via emit('connection')", async () => {
+    // STARTTLS-style: a plain net.Server accepts the connection and hands it to
+    // a tls.Server, which wraps it. The rejected handshake has to release the
+    // connection from the net.Server that accepted it.
+    const tlsServer = createServer({ ...COMMON_CERT, requestCert: true, rejectUnauthorized: true });
+    tlsServer.on("secureConnection", s => s.destroy(new Error("unexpected secureConnection")));
+    const { rawServer, port, accepted } = await listenRaw(raw => {
+      const observed = Promise.withResolvers<Accepted>();
+      // The wrap is only reachable through 'tlsClientError', which is emitted
+      // right before it is destroyed: observe it from inside the listener.
+      tlsServer.once("tlsClientError", (err: Error & { code?: string }, wrap) => {
+        observed.resolve(observe(raw, wrap, [`tlsClientError ${err.code} raw.destroyed=${raw.destroyed}`]));
+      });
+      tlsServer.emit("connection", raw);
+      return observed.promise;
+    });
+    // The client presents a self-signed certificate the server has no CA for.
+    const client = connect({ port, host: "127.0.0.1", ...COMMON_CERT, rejectUnauthorized: false });
+    client.on("error", () => {});
+    try {
+      await expectReleased(rawServer, await accepted, [
+        "tlsClientError DEPTH_ZERO_SELF_SIGNED_CERT raw.destroyed=false",
+        "wrap error DEPTH_ZERO_SELF_SIGNED_CERT raw.destroyed=false",
+        "raw close hadError=false",
+        "wrap close hadError=true raw.destroyed=true",
+      ]);
+    } finally {
+      client.destroy();
+      rawServer.close();
+      tlsServer.close();
+    }
+  });
+
+  it("when the wrap is destroyed in the tick it was created, before the socket was adopted", async () => {
+    const { rawServer, port, accepted } = await listenRaw(raw => {
+      const wrap = new TLSSocket(raw, { isServer: true, ...COMMON_CERT });
+      const conn = observe(raw, wrap);
+      wrap.destroy();
+      return conn;
+    });
+    const client = net.connect(port, "127.0.0.1");
+    client.on("error", () => {});
+    const clientClosed = new Promise<void>(resolve => client.once("close", () => resolve()));
+    try {
+      const conn = await accepted;
+      await conn.settled;
+      // The raw socket still owned the fd, so its 'close' follows its handle
+      // close, a later phase than the handle-less wrap's 'close': the order of
+      // the two is not part of the contract here, the set of events is.
+      conn.events.sort();
+      await expectReleased(rawServer, conn, [
+        "raw close hadError=false",
+        "wrap close hadError=false raw.destroyed=true",
+      ]);
+      // Destroying the raw socket is also what closes the connection the peer
+      // is holding.
+      await clientClosed;
+    } finally {
+      client.destroy();
+      rawServer.close();
+    }
+  });
+
+  it("when the wrap runs the TLS engine over a socket that has no handle", async () => {
+    const raw = new net.Socket();
+    const wrap = new TLSSocket(raw, { isServer: true, ...COMMON_CERT });
+    const { events, settled } = observe(raw, wrap);
+    wrap.destroy();
+    await settled;
+    expect(events).toEqual(["raw close hadError=false", "wrap close hadError=false raw.destroyed=true"]);
+  });
+});
+
 it("exposes the server-side peer verification result via socket.ssl.verifyError()", async () => {
   // Node's server path consults the same TLSWrap.verifyError() that clients
   // use, so the shim must be populated for server sockets too: