From 5f2b05932143859d9a1e99b047d121ac22f7c760 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 05:30:02 +0000 Subject: [PATCH 1/4] node:net: emit 'close' for a deferred destroy whose handle detached in the meantime Socket.prototype._destroy defers the handle close to a microtask when _closeAfterHandlingError is set (server-side TLS handshake failures), and closeSocketHandle re-read this._handle when that microtask ran. When the server-side TLS engine runs over a stream (upgradeDuplexToTLS: a wrapped connection with unflushed writes, or a generic Duplex), the engine dispatches its close callback right after the failed handshake, inside the same stream event, and that callback detaches _handle before the microtask runs. The deferred close then found no handle and the wrap never emitted 'close'. Capture the handle in _destroy and pass it to closeSocketHandle, so the deferred close tears down and reports the handle destroy() found regardless of what detached it since. Closing a handle that already closed natively is a no-op. --- src/js/node/net.ts | 45 +++++----- test/js/node/tls/node-tls-server.test.ts | 105 +++++++++++++++++++++++ 2 files changed, 130 insertions(+), 20 deletions(-) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 58e05eb224ee..b65239e0ddaa 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -2197,14 +2197,19 @@ Socket.prototype._destroy = function _destroy(err, callback) { } else if (this._closeAfterHandlingError) { // Enqueue closing the socket as a microtask, so that the socket can be // accessible when an `error` event is handled in the `next tick queue`. - queueMicrotask(() => closeSocketHandle(this, isException, true)); + // Close the handle captured above rather than re-reading _handle: when + // the TLS engine runs over a stream (upgradeDuplexToTLS) its close + // callback fires right after the failed handshake that brought us here + // and detaches _handle before this microtask runs. Nothing else emits + // 'close' once _destroy has run, so the deferred close must not bail. + queueMicrotask(() => closeSocketHandle(this, currentHandle, isException, true)); } else if (currentHandle[kAdoptedTLSRaw]) { // Shared-fd TLS pair: defer the close two check-phase turns // (test-tls-socket-close); see closeAdoptedTLSRawNT. currentHandle.pause?.(); setImmediate(closeAdoptedTLSRawNT, currentHandle, this, isException); } else { - closeSocketHandle(this, isException); + closeSocketHandle(this, currentHandle, isException); } if (!this._closeAfterHandlingError) { @@ -4272,25 +4277,25 @@ function initSocketHandle(self) { // intercepts close on `socket._handle` and invokes it, so always pass one. function onSocketHandleClosed() {} -function closeSocketHandle(self, isException, isCleanupPending = false) { - const handle = self._handle; - $debug("closeSocketHandle", isException, isCleanupPending, !!handle); - if (handle) { - handle.close(onSocketHandleClosed); - setImmediate(() => { - $debug("emit close", isCleanupPending); - self.emit("close", isException); - if (isCleanupPending) { - // A second destroy() before this runs clears self._handle, and a - // re-attach replaces it - only tear down the handle captured here. - handle.onread = noop; - if (self._handle === handle) { - self._handle = null; - self._sockname = null; - } +// Closes the handle _destroy found on the socket and emits 'close' for it. The +// handle may have closed natively and detached itself (self._handle is null by +// now) in the meantime; closing it again is a no-op and 'close' is still owed. +function closeSocketHandle(self, handle, isException, isCleanupPending = false) { + $debug("closeSocketHandle", isException, isCleanupPending); + handle.close(onSocketHandleClosed); + setImmediate(() => { + $debug("emit close", isCleanupPending); + self.emit("close", isException); + if (isCleanupPending) { + // A second destroy() before this runs clears self._handle, and a + // re-attach replaces it - only tear down the handle captured here. + handle.onread = noop; + if (self._handle === handle) { + self._handle = null; + self._sockname = null; } - }); - } + } + }); } // Reformat a native listen error to Node's "listen : " diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index dc5623688e61..7295b2098cac 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -5,6 +5,7 @@ import https from "https"; import net, { AddressInfo } from "net"; import { createTest } from "node-harness"; import { once } from "node:events"; +import { Duplex } from "node:stream"; import { tmpdir } from "os"; import { join } from "path"; import type { PeerCertificate } from "tls"; @@ -2019,6 +2020,110 @@ it("destroys a server wrap whose socket was destroyed before the deferred upgrad } }); +// A server wrap adopts the connection's fd unless the connection still has +// unflushed plain writes or is not a net.Socket at all; then the TLS engine runs +// over the stream itself. Whichever way the wrap is driven, node destroys it +// with the handshake error, so 'error' is followed by 'close' reporting +// hadError: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L480-L488 +// The stream-level engine reports the failed handshake and its own close back +// to back from inside one stream event; the wrap's deferred teardown used to +// find its handle already gone by the time it ran and never emitted 'close'. +describe("server-side TLSSocket wrap handshake failure", () => { + // Accepts one plain connection, hands it to `wrap` (which must write a plain + // banner so the peer knows the wrap is in place), lets `peer` drive the + // client once that banner arrives, and resolves with the wrap's 'error' / + // 'close' events once 'close' has fired. + async function failHandshake( + wrap: (conn: net.Socket) => TLSSocket, + peer: (client: net.Socket) => void, + ): Promise { + const events: string[] = []; + const closed = Promise.withResolvers(); + let conn: net.Socket | undefined; + const rawServer = net.createServer(socket => { + conn = socket; + // Only the wrap's lifecycle is under test; the plain connection's own + // fate after the peer goes away is not. + conn.on("error", () => {}); + const wrapped = wrap(conn); + wrapped.on("error", () => events.push("error")); + wrapped.on("close", hadError => { + events.push(`close:${hadError}`); + closed.resolve(events); + }); + }); + let client: net.Socket | undefined; + try { + const listening = Promise.withResolvers(); + rawServer.once("listening", listening.resolve); + rawServer.once("error", listening.reject); + rawServer.listen(0, "127.0.0.1"); + await listening.promise; + client = net.connect((rawServer.address() as AddressInfo).port, "127.0.0.1"); + client.on("error", () => {}); + client.once("data", () => peer(client!)); + return await closed.promise; + } finally { + client?.destroy(); + conn?.destroy(); + rawServer.close(); + } + } + + const adoptingWrap = (conn: net.Socket) => { + conn.write("220 banner\r\n"); + return new TLSSocket(conn, { isServer: true, ...COMMON_CERT }); + }; + + const unflushedWriteWrap = (conn: net.Socket) => { + conn.cork(); + conn.write("220 banner\r\n"); + const wrapped = new TLSSocket(conn, { isServer: true, ...COMMON_CERT }); + conn.uncork(); + return wrapped; + }; + + const duplexWrap = (conn: net.Socket) => { + conn.write("220 banner\r\n"); + const transport = new Duplex({ + read() {}, + write(chunk, encoding, callback) { + conn.write(chunk, encoding, callback); + }, + final(callback) { + conn.end(callback); + }, + }); + conn.on("data", chunk => transport.push(chunk)); + conn.on("end", () => transport.push(null)); + return new TLSSocket(transport, { isServer: true, ...COMMON_CERT }); + }; + + const sendPlaintext = (client: net.Socket) => { + client.write("this is not a ClientHello\r\n"); + }; + + it("emits 'error' then 'close' with hadError when the wrap adopted the connection's fd", async () => { + expect(await failHandshake(adoptingWrap, sendPlaintext)).toEqual(["error", "close:true"]); + }); + + it("emits 'error' then 'close' with hadError when the connection had unflushed writes", async () => { + expect(await failHandshake(unflushedWriteWrap, sendPlaintext)).toEqual(["error", "close:true"]); + }); + + it("emits 'error' then 'close' with hadError when wrapping a generic Duplex", async () => { + expect(await failHandshake(duplexWrap, sendPlaintext)).toEqual(["error", "close:true"]); + }); + + it("emits 'close' when the peer disconnects mid-handshake and the connection had unflushed writes", async () => { + const events = await failHandshake(unflushedWriteWrap, client => client.destroy()); + // Both wrap flavors currently report the disconnect through 'error' (node + // ends the wrap without one); either way 'close' must follow, and its + // hadError flag must agree with whether an 'error' was emitted. + expect(events.at(-1)).toBe(`close:${events.includes("error")}`); + }); +}); + 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: From c19943cda7481502af64e11f8226b16c73d11894 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:36:18 +0000 Subject: [PATCH 2/4] node:net: cover the fd-backed handshake failures that lose 'close' too The lost 'close' is not specific to the stream-level TLS engine: the native close callback detaches _handle ahead of the deferred close whenever the failed handshake is dispatched with JS already on the stack, which an asynchronous SNICallback rejection (resumeSNI runs from the user's callback) does on a plain tls.Server and on an fd-adopting wrap as well. Describe that condition at the _destroy branch and add both fd-backed cases to the test matrix. closeAdoptedTLSRawNowNT was the same close-then-report sequence as closeSocketHandle now that the latter takes the handle; use it directly. --- src/js/node/net.ts | 19 +++--- test/js/node/tls/node-tls-server.test.ts | 76 ++++++++++++++++++++---- 2 files changed, 71 insertions(+), 24 deletions(-) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index b65239e0ddaa..721a935d4045 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -242,12 +242,8 @@ function emitCloseNT(self, hasError) { // Shared-fd TLS pair teardown: mirrors node's close ordering, where the // close-callbacks phase runs after the check phase (lib/net.js close path in // node v26.3.0), so destroy()-time setImmediates still see the pair alive. -function closeAdoptedTLSRawNT(handle, self, isException) { - setImmediate(closeAdoptedTLSRawNowNT, handle, self, isException); -} -function closeAdoptedTLSRawNowNT(handle, self, isException) { - handle.close(onSocketHandleClosed); - setImmediate(emitCloseNT, self, isException); +function closeAdoptedTLSRawNT(self, handle, isException) { + setImmediate(closeSocketHandle, self, handle, isException); } function detachSocket(self) { if (!self) self = this; @@ -2198,16 +2194,17 @@ Socket.prototype._destroy = function _destroy(err, callback) { // Enqueue closing the socket as a microtask, so that the socket can be // accessible when an `error` event is handled in the `next tick queue`. // Close the handle captured above rather than re-reading _handle: when - // the TLS engine runs over a stream (upgradeDuplexToTLS) its close - // callback fires right after the failed handshake that brought us here - // and detaches _handle before this microtask runs. Nothing else emits - // 'close' once _destroy has run, so the deferred close must not bail. + // the failed handshake was dispatched with JS already on the stack (the + // stream-level TLS engine fed from a 'data' listener, resumeSNI from an + // asynchronous SNICallback completion), the native close callback runs + // right behind it, before any microtask, and detaches _handle. Nothing + // else emits 'close' once _destroy has run, so this must not bail. queueMicrotask(() => closeSocketHandle(this, currentHandle, isException, true)); } else if (currentHandle[kAdoptedTLSRaw]) { // Shared-fd TLS pair: defer the close two check-phase turns // (test-tls-socket-close); see closeAdoptedTLSRawNT. currentHandle.pause?.(); - setImmediate(closeAdoptedTLSRawNT, currentHandle, this, isException); + setImmediate(closeAdoptedTLSRawNT, this, currentHandle, isException); } else { closeSocketHandle(this, currentHandle, isException); } diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index 7295b2098cac..4fa9fe0966a6 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -2020,15 +2020,18 @@ it("destroys a server wrap whose socket was destroyed before the deferred upgrad } }); -// A server wrap adopts the connection's fd unless the connection still has -// unflushed plain writes or is not a net.Socket at all; then the TLS engine runs -// over the stream itself. Whichever way the wrap is driven, node destroys it -// with the handshake error, so 'error' is followed by 'close' reporting -// hadError: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L480-L488 -// The stream-level engine reports the failed handshake and its own close back -// to back from inside one stream event; the wrap's deferred teardown used to -// find its handle already gone by the time it ran and never emitted 'close'. -describe("server-side TLSSocket wrap handshake failure", () => { +// A failed server-side handshake destroys the socket with the error, so after +// 'error' (tlsClientError on a tls.Server) node emits 'close' reporting hadError: +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L480-L488 +// That destroy defers closing the handle to a microtask. Whenever the failure is +// dispatched with JS already on the stack, the native close callback follows it +// before that microtask runs: the stream-level TLS engine (a wrapped connection +// with unflushed writes, or a generic Duplex) is fed from a 'data' listener, and +// an asynchronous SNICallback rejection resumes the handshake from the user's +// callback. The deferred teardown used to find the handle already detached and +// never emitted 'close'. A failure dispatched straight from the event loop (the +// fd-adopting wrap fed bad bytes) never had the problem and pins that ordering. +describe("server-side handshake failure emits 'close'", () => { // Accepts one plain connection, hands it to `wrap` (which must write a plain // banner so the peer knows the wrap is in place), lets `peer` drive the // client once that banner arrives, and resolves with the wrap's 'error' / @@ -2070,15 +2073,17 @@ describe("server-side TLSSocket wrap handshake failure", () => { } } - const adoptingWrap = (conn: net.Socket) => { + const serverOptions = (extra: tls.TlsOptions = {}) => ({ isServer: true, ...COMMON_CERT, ...extra }); + + const adoptingWrap = (conn: net.Socket, extra?: tls.TlsOptions) => { conn.write("220 banner\r\n"); - return new TLSSocket(conn, { isServer: true, ...COMMON_CERT }); + return new TLSSocket(conn, serverOptions(extra)); }; const unflushedWriteWrap = (conn: net.Socket) => { conn.cork(); conn.write("220 banner\r\n"); - const wrapped = new TLSSocket(conn, { isServer: true, ...COMMON_CERT }); + const wrapped = new TLSSocket(conn, serverOptions()); conn.uncork(); return wrapped; }; @@ -2096,13 +2101,25 @@ describe("server-side TLSSocket wrap handshake failure", () => { }); conn.on("data", chunk => transport.push(chunk)); conn.on("end", () => transport.push(null)); - return new TLSSocket(transport, { isServer: true, ...COMMON_CERT }); + return new TLSSocket(transport, serverOptions()); }; const sendPlaintext = (client: net.Socket) => { client.write("this is not a ClientHello\r\n"); }; + // Rejects from a later turn, so the handshake is suspended and then resumed + // (and failed) from inside this callback rather than from the event loop. + const rejectingSNI: tls.TlsOptions = { + SNICallback: (_servername, cb) => { + setImmediate(cb, new Error("unknown servername")); + }, + }; + + const startTLSClient = (client: net.Socket) => { + connect({ socket: client, servername: "rejected.example.com", rejectUnauthorized: false }).on("error", () => {}); + }; + it("emits 'error' then 'close' with hadError when the wrap adopted the connection's fd", async () => { expect(await failHandshake(adoptingWrap, sendPlaintext)).toEqual(["error", "close:true"]); }); @@ -2122,6 +2139,39 @@ describe("server-side TLSSocket wrap handshake failure", () => { // hadError flag must agree with whether an 'error' was emitted. expect(events.at(-1)).toBe(`close:${events.includes("error")}`); }); + + it("emits 'error' then 'close' with hadError when an asynchronous SNICallback rejects an fd-adopting wrap", async () => { + const events = await failHandshake(conn => adoptingWrap(conn, rejectingSNI), startTLSClient); + expect(events).toEqual(["error", "close:true"]); + }); + + it("tls.Server emits 'close' with hadError on the socket it reported through tlsClientError when an asynchronous SNICallback rejects", async () => { + const server = createServer({ ...COMMON_CERT, ...rejectingSNI }); + const closed = Promise.withResolvers<{ message: string; hadError: boolean }>(); + server.on("tlsClientError", (err, socket) => { + socket.on("close", hadError => closed.resolve({ message: err.message, hadError })); + }); + server.on("secureConnection", () => closed.reject(new Error("secureConnection must not fire"))); + let client: TLSSocket | undefined; + try { + const listening = Promise.withResolvers(); + server.once("listening", listening.resolve); + server.once("error", listening.reject); + server.listen(0, "127.0.0.1"); + await listening.promise; + client = connect({ + port: (server.address() as AddressInfo).port, + host: "127.0.0.1", + servername: "rejected.example.com", + rejectUnauthorized: false, + }); + client.on("error", () => {}); + expect(await closed.promise).toEqual({ message: "unknown servername", hadError: true }); + } finally { + client?.destroy(); + server.close(); + } + }); }); it("exposes the server-side peer verification result via socket.ssl.verifyError()", async () => { From 26773c6319498ff3ccf3f8bc5a4042300bd023e2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:46:04 +0000 Subject: [PATCH 3/4] node:net: shorten the closeSocketHandle comments --- src/js/node/net.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 721a935d4045..98e95bc26735 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -2193,12 +2193,9 @@ Socket.prototype._destroy = function _destroy(err, callback) { } else if (this._closeAfterHandlingError) { // Enqueue closing the socket as a microtask, so that the socket can be // accessible when an `error` event is handled in the `next tick queue`. - // Close the handle captured above rather than re-reading _handle: when - // the failed handshake was dispatched with JS already on the stack (the - // stream-level TLS engine fed from a 'data' listener, resumeSNI from an - // asynchronous SNICallback completion), the native close callback runs - // right behind it, before any microtask, and detaches _handle. Nothing - // else emits 'close' once _destroy has run, so this must not bail. + // A handshake failure dispatched from inside a JS-initiated native call + // (stream-level TLS engine, resumeSNI) is followed by the native close, + // which detaches _handle, before this microtask runs; pass the handle. queueMicrotask(() => closeSocketHandle(this, currentHandle, isException, true)); } else if (currentHandle[kAdoptedTLSRaw]) { // Shared-fd TLS pair: defer the close two check-phase turns @@ -4274,9 +4271,8 @@ function initSocketHandle(self) { // intercepts close on `socket._handle` and invokes it, so always pass one. function onSocketHandleClosed() {} -// Closes the handle _destroy found on the socket and emits 'close' for it. The -// handle may have closed natively and detached itself (self._handle is null by -// now) in the meantime; closing it again is a no-op and 'close' is still owed. +// `handle` is the one _destroy found; close() on one the native side already +// closed is a no-op, and 'close' is emitted either way. function closeSocketHandle(self, handle, isException, isCleanupPending = false) { $debug("closeSocketHandle", isException, isCleanupPending); handle.close(onSocketHandleClosed); @@ -4284,8 +4280,8 @@ function closeSocketHandle(self, handle, isException, isCleanupPending = false) $debug("emit close", isCleanupPending); self.emit("close", isException); if (isCleanupPending) { - // A second destroy() before this runs clears self._handle, and a - // re-attach replaces it - only tear down the handle captured here. + // The native close may have detached self._handle by now, or a re-attach + // replaced it; only tear down the handle captured here. handle.onread = noop; if (self._handle === handle) { self._handle = null; From 14f056cae71233ca4bd8ff32477c08006dfed683 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:56:03 +0000 Subject: [PATCH 4/4] node:net: drop the redundant closeSocketHandle comments --- src/js/node/net.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 98e95bc26735..510192d35c20 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -4271,8 +4271,6 @@ function initSocketHandle(self) { // intercepts close on `socket._handle` and invokes it, so always pass one. function onSocketHandleClosed() {} -// `handle` is the one _destroy found; close() on one the native side already -// closed is a no-op, and 'close' is emitted either way. function closeSocketHandle(self, handle, isException, isCleanupPending = false) { $debug("closeSocketHandle", isException, isCleanupPending); handle.close(onSocketHandleClosed); @@ -4280,8 +4278,7 @@ function closeSocketHandle(self, handle, isException, isCleanupPending = false) $debug("emit close", isCleanupPending); self.emit("close", isException); if (isCleanupPending) { - // The native close may have detached self._handle by now, or a re-attach - // replaced it; only tear down the handle captured here. + // self._handle may have been detached or replaced since. handle.onread = noop; if (self._handle === handle) { self._handle = null;