Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/bun.js/api/bun/socket/Listener.zig
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,12 @@
}
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;
Expand Down Expand Up @@ -652,8 +657,9 @@
if (isNamedPipe) {
default_data.ensureStillAlive();

const handlers_ptr = bun.handleOom(handlers.vm.allocator.create(Handlers));
handlers_ptr.* = handlers.*;
handlers_ptr.mode = .client;

Check notice on line 662 in src/bun.js/api/bun/socket/Listener.zig

View check run for this annotation

Claude / Claude Code Review

Pre-existing: connection.unix path leaks on Windows named-pipe non-TLS Bun.connect

Pre-existing (not introduced here), but since this PR is hardening lifecycle in this exact `if (isNamedPipe)` block: the **non-TLS** arm just below (~lines 731–775) never adopts `connection` — both the `prev_maybe_tcp` reuse path (`bun.assert(prev.connection == null)`, no assignment) and `TCPSocket.new(.{ .connection = null, ... })` drop the heap-owned `connection.unix` slice on the floor, and the branch returns `promise_value` so the `errdefer connection.deinit()` never fires. The TLS arm and t
Comment thread
claude[bot] marked this conversation as resolved.

var promise = jsc.JSPromise.create(globalObject);
const promise_value = promise.toJS();
Expand Down
113 changes: 113 additions & 0 deletions test/js/bun/net/socket.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});
});
Loading