From 28be36bed9740270fac7d0d66243f2b16ca6c584 Mon Sep 17 00:00:00 2001 From: robobun Date: Sun, 3 May 2026 00:01:22 +0000 Subject: [PATCH 1/2] socket: set Handlers.mode=.client for Windows named-pipe Bun.connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connectInner() passed is_server=true to SocketConfig.fromJS (regressed in 4a06991d3b; it was false before), so the handlers start with .mode=.server. The non-pipe path overrides that to .client right after copying into the standalone allocator.create(Handlers) block, but the Windows named-pipe branch did not. With .mode left as .server, Handlers.markInactive() on close/fail takes the server branch and does @fieldParentPtr("handlers", this) to reach an enclosing Listener — but the block is a bare Handlers, so reading listen_socket.listener falls past the allocation (heap-buffer-overflow under ASAN) and the .client branch that frees the block is skipped (leak). Restore is_server=false at the connectInner call site and mirror the defensive .mode=.client override on the named-pipe branch to match the non-pipe path. --- src/bun.js/api/bun/socket/Listener.zig | 8 +- test/js/bun/net/socket.test.ts | 113 +++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/bun.js/api/bun/socket/Listener.zig b/src/bun.js/api/bun/socket/Listener.zig index d0a929572392..bada4a51c02d 100644 --- a/src/bun.js/api/bun/socket/Listener.zig +++ b/src/bun.js/api/bun/socket/Listener.zig @@ -561,7 +561,12 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock } const vm = globalObject.bunVM(); - var socket_config = try SocketConfig.fromJS(vm, opts, globalObject, true); + // is_server=false: this is the client connect path. Handlers.mode must be + // .client so markInactive() takes the allocator.destroy branch — the + // .server branch does @fieldParentPtr("handlers", this) to reach a + // Listener, but here handlers live in a standalone allocator.create() + // block (see below), so that would read past the allocation. + var socket_config = try SocketConfig.fromJS(vm, opts, globalObject, false); defer socket_config.deinitExcludingHandlers(); const handlers = &socket_config.handlers; @@ -654,6 +659,7 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock const handlers_ptr = bun.handleOom(handlers.vm.allocator.create(Handlers)); handlers_ptr.* = handlers.*; + handlers_ptr.mode = .client; var promise = jsc.JSPromise.create(globalObject); const promise_value = promise.toJS(); diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 76999d26bdcc..f58c85f81486 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -949,3 +949,116 @@ it("TLS client: flush() after end() does not double-teardown before deferred onC expect(stdout.trim()).toBe("OK"); expect(exitCode).toBe(0); }, 30_000); // debug subprocess startup + ASAN symbolication on failure is slow + +// Bun.connect() on a Windows named pipe takes a dedicated early branch in +// Listener.connectInner that heap-allocates a standalone Handlers block. That +// block's `.mode` must be `.client` so Handlers.markInactive() destroys it on +// close; the `.server` path does `@fieldParentPtr("handlers", ...)` expecting +// a surrounding Listener struct and would read past the standalone +// allocation (heap-buffer-overflow under ASAN) and leak the block. +describe.skipIf(!isWindows)("Bun.connect named-pipe client Handlers lifecycle", () => { + it("open → close cleans up without reading past the Handlers allocation", async () => { + const src = /* js */ ` + const pipe = "\\\\\\\\.\\\\pipe\\\\bun-test-connect-" + Math.random().toString(36).slice(2); + + const closed = Promise.withResolvers(); + const opened = Promise.withResolvers(); + + using server = Bun.listen({ + unix: pipe, + socket: { + data() {}, + open(s) { s.end(); }, + close() {}, + error() {}, + }, + }); + + const client = await Bun.connect({ + unix: pipe, + socket: { + data() {}, + open() { opened.resolve(); }, + close() { closed.resolve(); }, + error() {}, + }, + }); + + await opened.promise; + client.end(); + await closed.promise; + server.stop(true); + + Bun.gc(true); + console.log("OK"); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + timeout: 15_000, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ + stdout: stdout.trim(), + stderr: stderr.trim(), + exitCode, + signalCode: proc.signalCode ?? null, + }).toMatchObject({ + stdout: "OK", + exitCode: 0, + signalCode: null, + }); + }); + + it("failed connect to a non-existent pipe rejects and cleans up", async () => { + const src = /* js */ ` + const pipe = "\\\\\\\\.\\\\pipe\\\\bun-test-missing-" + Math.random().toString(36).slice(2); + + let rejected = false; + await Bun.connect({ + unix: pipe, + socket: { + data() {}, + open() {}, + close() {}, + connectError() {}, + error() {}, + }, + }).catch(() => { rejected = true; }); + + if (!rejected) { + console.error("expected Bun.connect to reject for a non-existent pipe"); + process.exit(1); + } + + Bun.gc(true); + console.log("OK"); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + timeout: 15_000, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ + stdout: stdout.trim(), + stderr: stderr.trim(), + exitCode, + signalCode: proc.signalCode ?? null, + }).toMatchObject({ + stdout: "OK", + exitCode: 0, + signalCode: null, + }); + }); +}); From 7dd9d4056278785d94a90571e666e438a5821ba6 Mon Sep 17 00:00:00 2001 From: robobun Date: Sun, 3 May 2026 03:39:33 +0000 Subject: [PATCH 2/2] socket: adopt connection into non-TLS named-pipe TCPSocket The non-TLS Windows named-pipe arm in connectInner() was the only construction path that left tcp.connection = null instead of adopting the heap-owned connection.unix slice (the TLS arm right above and the non-pipe arm below both adopt it). On the success return the errdefer doesn't fire, so the duped pipe-path bytes leaked per Bun.connect({unix: pipe}) on Windows. Mirror the TLS arm: deinit any old connection on the reuse path and assign connection so TCPSocket deinit frees it. --- src/bun.js/api/bun/socket/Listener.zig | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/bun.js/api/bun/socket/Listener.zig b/src/bun.js/api/bun/socket/Listener.zig index bada4a51c02d..fcfa16f68eba 100644 --- a/src/bun.js/api/bun/socket/Listener.zig +++ b/src/bun.js/api/bun/socket/Listener.zig @@ -737,7 +737,14 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock } prev.handlers = handlers_ptr; bun.assert(prev.socket.socket == .detached); - bun.assert(prev.connection == null); + // Adopt `connection` (heap-owned for .unix) so the socket's + // deinit frees it; matches the TLS arm above and the + // non-pipe arm below. Previously `.connection = null` + // dropped the duped pipe-path bytes on the floor. + if (prev.connection) |old_connection| { + old_connection.deinit(); + } + prev.connection = connection; bun.assert(prev.protos == null); bun.assert(prev.server_name == null); break :blk prev; @@ -745,7 +752,7 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock .ref_count = .init(), .handlers = handlers_ptr, .socket = TCPSocket.Socket.detached, - .connection = null, + .connection = connection, .protos = null, .server_name = null, });