Compare commits

...

6 Commits

Author SHA1 Message Date
Jarred Sumner
e527de8c01 a lot of the current code in here needs to be rewritten. this barely makes it better. 2025-09-05 01:36:33 -07:00
Jarred Sumner
df9ecd7d12 Merge branch 'main' into claude/fix-unix-socket-cleanup 2025-09-04 22:19:04 -07:00
Jarred Sumner
a6f18dd61f Merge branch 'main' into claude/fix-unix-socket-cleanup 2025-09-04 19:20:10 -07:00
autofix-ci[bot]
eb94e2eee3 [autofix.ci] apply automated fixes 2025-09-04 22:32:36 +00:00
Jarred Sumner
a7a37896ff Merge branch 'main' into claude/fix-unix-socket-cleanup 2025-09-04 15:30:42 -07:00
Claude Bot
d3770a7f6a Fix Unix domain socket cleanup on listener.stop()
When calling listener.stop() on a Unix domain socket, the socket file
was not being cleaned up from the filesystem. This follows the same
approach as libuv to fix the issue.

The fix unlinks the socket file BEFORE closing the file descriptor to
avoid race conditions where another process could create a new socket
with the same name between close and unlink operations.

Reference: https://github.com/libuv/libuv/blob/v1.x/src/unix/pipe.c#L179-L193
2025-09-04 09:18:10 +00:00
5 changed files with 267 additions and 46 deletions

View File

@@ -108,24 +108,24 @@ pub fn NewSocket(comptime ssl: bool) type {
}
}
pub fn doConnect(this: *This, connection: Listener.UnixOrHost) !void {
pub fn doConnect(this: *This, connection: *const Listener.UnixOrHost) !void {
bun.assert(this.socket_context != null);
this.ref();
errdefer this.deref();
switch (connection) {
.host => |c| {
switch (connection.*) {
.host => |*c| {
this.socket = try This.Socket.connectAnon(
c.host,
c.host.slice(),
c.port,
this.socket_context.?,
this,
this.flags.allow_half_open,
);
},
.unix => |u| {
.unix => |*u| {
this.socket = try This.Socket.connectUnixAnon(
u,
u.slice(),
this.socket_context.?,
this,
this.flags.allow_half_open,
@@ -426,7 +426,7 @@ pub fn NewSocket(comptime ssl: bool) type {
if (connection == .host) {
const host = connection.host.host;
if (host.len > 0) {
const host__ = bun.handleOom(default_allocator.dupeZ(u8, host));
const host__ = bun.handleOom(default_allocator.dupeZ(u8, host.slice()));
defer default_allocator.free(host__);
ssl_ptr.setHostname(host__);
}
@@ -599,7 +599,7 @@ pub fn NewSocket(comptime ssl: bool) type {
pub fn onClose(this: *This, _: Socket, err: c_int, _: ?*anyopaque) void {
jsc.markBinding(@src());
const handlers = this.getHandlers();
log("onClose {s}", .{if (handlers.is_server) "S" else "C"});
log("onClose {s} {?}", .{ if (handlers.is_server) "S" else "C", this.connection });
this.detachNativeCallback();
this.socket.detach();
defer this.deref();
@@ -1309,9 +1309,9 @@ pub fn NewSocket(comptime ssl: bool) type {
default_allocator.free(server_name);
}
if (this.connection) |connection| {
this.connection = null;
if (this.connection) |*connection| {
connection.deinit();
this.connection = null;
}
if (this.socket_context) |socket_context| {
this.socket_context = null;

View File

@@ -278,7 +278,7 @@ pub const SocketConfig = struct {
if (try opts.getStringish(globalObject, "unix")) |unix_socket| {
defer unix_socket.deref();
hostname_or_unix = try unix_socket.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator);
hostname_or_unix = unix_socket.toUTF8(bun.default_allocator);
if (strings.hasPrefixComptime(hostname_or_unix.slice(), "file://") or strings.hasPrefixComptime(hostname_or_unix.slice(), "unix://") or strings.hasPrefixComptime(hostname_or_unix.slice(), "sock://")) {
// The memory allocator relies on the pointer address to
@@ -313,6 +313,7 @@ pub const SocketConfig = struct {
defer hostname.deref();
var port_value = try opts.get(globalObject, "port") orelse JSValue.zero;
hostname_or_unix.deinit();
hostname_or_unix = try hostname.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator);
if (port_value.isEmptyOrUndefinedOrNull() and hostname_or_unix.len > 0) {

View File

@@ -36,24 +36,30 @@ pub fn setData(this: *Listener, globalObject: *jsc.JSGlobalObject, value: jsc.JS
}
pub const UnixOrHost = union(enum) {
unix: []const u8,
unix: ZigString.Slice,
host: struct {
host: []const u8,
host: ZigString.Slice,
port: u16,
pub fn deinit(this: *@This()) void {
this.host.deinit();
}
},
fd: bun.FileDescriptor,
pub fn clone(this: UnixOrHost) UnixOrHost {
switch (this) {
.unix => |u| {
pub const empty: UnixOrHost = .{ .unix = .empty };
pub fn clone(this: *const UnixOrHost) UnixOrHost {
switch (this.*) {
.unix => |*u| {
return .{
.unix = bun.handleOom(bun.default_allocator.dupe(u8, u)),
.unix = u.toOwned(bun.default_allocator) catch |e| bun.handleOom(e),
};
},
.host => |h| {
.host => |*h| {
return .{
.host = .{
.host = bun.handleOom(bun.default_allocator.dupe(u8, h.host)),
.host = h.host.toOwned(bun.default_allocator) catch |e| bun.handleOom(e),
.port = this.host.port,
},
};
@@ -62,13 +68,13 @@ pub const UnixOrHost = union(enum) {
}
}
pub fn deinit(this: UnixOrHost) void {
switch (this) {
.unix => |u| {
bun.default_allocator.free(u);
pub fn deinit(this: *UnixOrHost) void {
switch (this.*) {
.unix => |*u| {
u.deinit();
},
.host => |h| {
bun.default_allocator.free(h.host);
.host => |*h| {
h.deinit();
},
.fd => {}, // this is an integer
}
@@ -113,6 +119,7 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
var socket_config = try SocketConfig.fromJS(vm, opts, globalObject, true);
var hostname_or_unix = socket_config.hostname_or_unix;
defer hostname_or_unix.deinit();
const port = socket_config.port;
var ssl = socket_config.ssl;
var handlers = socket_config.handlers;
@@ -127,9 +134,11 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
if (port == null) {
// we check if the path is a named pipe otherwise we try to connect using AF_UNIX
const slice = hostname_or_unix.slice();
var buf: bun.PathBuffer = undefined;
var buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(buf);
if (normalizePipeName(slice, buf[0..])) |pipe_name| {
const connection: Listener.UnixOrHost = .{ .unix = bun.handleOom(hostname_or_unix.cloneIfNeeded(bun.default_allocator)).slice() };
const connection: Listener.UnixOrHost = .{ .unix = hostname_or_unix };
hostname_or_unix = .empty;
if (ssl_enabled) {
if (ssl.?.protos) |p| {
protos = p[0..ssl.?.protos_len];
@@ -187,7 +196,6 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
var err = globalObject.createErrorInstance("Failed to listen on {s}:{d}", .{ hostname_or_unix.slice(), port orelse 0 });
defer {
socket_config.handlers.unprotect();
hostname_or_unix.deinit();
}
const errno = @intFromEnum(bun.sys.getErrno(@as(c_int, -1)));
@@ -242,15 +250,18 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
}
var connection: Listener.UnixOrHost = if (port) |port_| .{
.host = .{ .host = bun.handleOom(hostname_or_unix.cloneIfNeeded(bun.default_allocator)).slice(), .port = port_ },
.host = .{ .host = hostname_or_unix, .port = port_ },
} else if (socket_config.fd) |fd| .{ .fd = fd } else .{
.unix = bun.handleOom(hostname_or_unix.cloneIfNeeded(bun.default_allocator)).slice(),
.unix = hostname_or_unix,
};
hostname_or_unix = .empty;
defer connection.deinit();
var errno: c_int = 0;
const listen_socket: *uws.ListenSocket = brk: {
switch (connection) {
.host => |c| {
const host = bun.handleOom(bun.default_allocator.dupeZ(u8, c.host));
const host = bun.handleOom(bun.default_allocator.dupeZ(u8, c.host.slice()));
defer bun.default_allocator.free(host);
const socket = socket_context.listen(ssl_enabled, host.ptr, c.port, socket_flags, 8, &errno);
@@ -261,7 +272,7 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
break :brk socket;
},
.unix => |u| {
const host = bun.handleOom(bun.default_allocator.dupeZ(u8, u));
const host = bun.handleOom(bun.default_allocator.dupeZ(u8, u.slice()));
defer bun.default_allocator.free(host);
break :brk socket_context.listenUnix(ssl_enabled, host, host.len, socket_flags, 8, &errno);
},
@@ -278,7 +289,6 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
}
} orelse {
defer {
hostname_or_unix.deinit();
socket_context.free(ssl_enabled);
}
@@ -298,7 +308,11 @@ pub fn listen(globalObject: *jsc.JSGlobalObject, opts: JSValue) bun.JSError!JSVa
var socket = Listener{
.handlers = handlers,
.connection = connection,
.connection = brk: {
const old = connection;
connection = .empty;
break :brk old;
},
.ssl = ssl_enabled,
.socket_context = socket_context,
.listener = .{ .uws = listen_socket },
@@ -437,6 +451,23 @@ pub fn stop(this: *Listener, _: *jsc.JSGlobalObject, callframe: *jsc.CallFrame)
fn doStop(this: *Listener, force_close: bool) void {
if (this.listener == .none) return;
const listener = this.listener;
// Unlink Unix socket file BEFORE closing the socket to avoid race conditions
// (same approach as libuv - see uv__pipe_close in deps/uv/src/unix/pipe.c)
if (!Environment.isWindows) {
if (this.connection == .unix) {
const path = this.connection.unix;
// Don't unlink abstract sockets (those starting with '\0')
if (path.slice().len > 0 and path.slice()[0] != 0) {
const path_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(path_buf);
@memcpy(path_buf.ptr[0..path.slice().len], path.slice());
path_buf.ptr[path.slice().len] = 0;
_ = bun.sys.unlink(path_buf.ptr[0..path.len :0]);
}
}
}
defer switch (listener) {
.uws => |socket| socket.close(this.ssl),
.namedPipe => |namedPipe| if (Environment.isWindows) namedPipe.closePipeAndDeinit(),
@@ -514,14 +545,14 @@ pub fn getUnix(this: *Listener, globalObject: *jsc.JSGlobalObject) JSValue {
return .js_undefined;
}
return ZigString.init(this.connection.unix).withEncoding().toJS(globalObject);
return this.connection.unix.toZigString().withEncoding().toJS(globalObject);
}
pub fn getHostname(this: *Listener, globalObject: *jsc.JSGlobalObject) JSValue {
if (this.connection != .host) {
return .js_undefined;
}
return ZigString.init(this.connection.host.host).withEncoding().toJS(globalObject);
return this.connection.host.host.toZigString().withEncoding().toJS(globalObject);
}
pub fn getPort(this: *Listener, _: *jsc.JSGlobalObject) JSValue {
@@ -560,6 +591,7 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock
const socket_config = try SocketConfig.fromJS(vm, opts, globalObject, false);
var hostname_or_unix = socket_config.hostname_or_unix;
defer hostname_or_unix.deinit();
const port = socket_config.port;
var ssl = socket_config.ssl;
var handlers = socket_config.handlers;
@@ -579,20 +611,25 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock
break :blk .{ .fd = fd };
}
}
defer {
hostname_or_unix = .empty;
}
if (port) |_| {
break :blk .{ .host = .{ .host = bun.handleOom(hostname_or_unix.cloneIfNeeded(bun.default_allocator)).slice(), .port = port.? } };
break :blk .{ .host = .{ .host = hostname_or_unix, .port = port.? } };
}
break :blk .{ .unix = bun.handleOom(hostname_or_unix.cloneIfNeeded(bun.default_allocator)).slice() };
break :blk .{ .unix = hostname_or_unix };
};
defer connection.deinit();
if (Environment.isWindows) {
var buf: bun.PathBuffer = undefined;
var buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(buf);
var pipe_name: ?[]const u8 = null;
const isNamedPipe = switch (connection) {
// we check if the path is a named pipe otherwise we try to connect using AF_UNIX
.unix => |slice| brk: {
pipe_name = normalizePipeName(slice, buf[0..]);
pipe_name = normalizePipeName(slice.slice(), buf[0..]);
break :brk (pipe_name != null);
},
.fd => |fd| brk: {
@@ -647,17 +684,19 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock
.server_name = server_name,
.socket_context = null,
});
connection = .empty;
TLSSocket.js.dataSetCached(tls.getThisValue(globalObject), globalObject, default_data);
tls.poll_ref.ref(handlers.vm);
tls.ref();
if (connection == .unix) {
if (tls.connection.? == .unix) {
const named_pipe = WindowsNamedPipeContext.connect(globalObject, pipe_name.?, ssl, .{ .tls = tls }) catch {
return promise_value;
};
tls.socket = TLSSocket.Socket.fromNamedPipe(named_pipe);
} else {
// fd
const named_pipe = WindowsNamedPipeContext.open(globalObject, connection.fd, ssl, .{ .tls = tls }) catch {
const named_pipe = WindowsNamedPipeContext.open(globalObject, tls.connection.?.fd, ssl, .{ .tls = tls }) catch {
return promise_value;
};
tls.socket = TLSSocket.Socket.fromNamedPipe(named_pipe);
@@ -719,7 +758,6 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock
.code = if (port == null) bun.String.static("ENOENT") else bun.String.static("ECONNREFUSED"),
};
handlers.unprotect();
connection.deinit();
return globalObject.throwValue(err.toErrorInstance(globalObject));
};
@@ -769,10 +807,11 @@ pub fn connectInner(globalObject: *jsc.JSGlobalObject, prev_maybe_tcp: ?*TCPSock
.server_name = server_name,
.socket_context = socket_context, // owns the socket context
});
connection = .empty;
socket.ref();
SocketType.js.dataSetCached(socket.getThisValue(globalObject), globalObject, default_data);
socket.flags.allow_half_open = socket_config.allowHalfOpen;
socket.doConnect(connection) catch {
socket.doConnect(&socket.connection.?) catch {
socket.handleConnectError(@intFromEnum(if (port == null) bun.sys.SystemErrno.ENOENT else bun.sys.SystemErrno.ECONNREFUSED));
return promise_value;
};
@@ -932,7 +971,9 @@ pub const WindowsNamedPipeListeningContext = if (Environment.isWindows) struct {
const slice_z = path[0 .. path.len - 1 :0];
this.uvPipe.listenNamedPipe(slice_z, backlog, this, onClientConnect).unwrap() catch return error.FailedToBindPipe;
} else {
var path_buf: bun.PathBuffer = undefined;
var path_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(path_buf);
// we need to null terminate the path
const len = @min(path.len, path_buf.len - 1);

View File

@@ -360,7 +360,7 @@ pub const ZigString = extern struct {
return !this.allocator.isNull();
}
pub fn toOwned(this: Slice, allocator: std.mem.Allocator) OOM!Slice {
pub fn toOwned(this: *const Slice, allocator: std.mem.Allocator) OOM!Slice {
const duped = try allocator.dupe(u8, this.ptr[0..this.len]);
return .{ .allocator = .init(allocator), .ptr = duped.ptr, .len = this.len };
}

View File

@@ -0,0 +1,179 @@
import { expect, test } from "bun:test";
import { isWindows } from "harness";
import { randomBytes } from "node:crypto";
import { existsSync, unlinkSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
test.skipIf(isWindows)("Unix domain socket file should be cleaned up when listener.stop() is called", () => {
// Generate a random socket path to avoid conflicts
const socketPath = join(tmpdir(), `bun_test_${randomBytes(8).toString("hex")}.sock`);
// Clean up any existing socket file
if (existsSync(socketPath)) {
unlinkSync(socketPath);
}
// Create a Unix socket listener
const listener = Bun.listen({
unix: socketPath,
socket: {
open(socket) {},
data(socket, data) {},
close(socket) {},
},
});
// Verify the socket file was created
expect(existsSync(socketPath)).toBe(true);
// Stop the listener
listener.stop();
// Verify the socket file was cleaned up
expect(existsSync(socketPath)).toBe(false);
});
test.skipIf(isWindows)("Unix domain socket file should be cleaned up when listener.stop(true) is called", () => {
// Generate a random socket path
const socketPath = join(tmpdir(), `bun_test_${randomBytes(8).toString("hex")}.sock`);
// Clean up any existing socket file
if (existsSync(socketPath)) {
unlinkSync(socketPath);
}
// Create a Unix socket listener
const listener = Bun.listen({
unix: socketPath,
socket: {
open(socket) {},
data(socket, data) {},
close(socket) {},
},
});
// Verify the socket file was created
expect(existsSync(socketPath)).toBe(true);
// Stop the listener with force=true
listener.stop(true);
// Verify the socket file was cleaned up
expect(existsSync(socketPath)).toBe(false);
});
test.skipIf(isWindows)("Abstract Unix domain sockets should not leave files (start with null byte)", () => {
// Abstract sockets start with a null byte and don't create filesystem entries
const abstractPath = "\0bun_test_abstract_" + randomBytes(8).toString("hex");
// Create an abstract Unix socket listener
const listener = Bun.listen({
unix: abstractPath,
socket: {
open(socket) {},
data(socket, data) {},
close(socket) {},
},
});
// Abstract sockets shouldn't create a file in the filesystem
// We can't really check this, but we can verify stop() doesn't crash
listener.stop();
// Test passes if no crash occurs
expect(true).toBe(true);
});
test.skipIf(isWindows)("Multiple Unix sockets cleanup", () => {
const sockets = [];
const paths = [];
// Create multiple Unix socket listeners
for (let i = 0; i < 3; i++) {
const socketPath = join(tmpdir(), `bun_test_multi_${i}_${randomBytes(4).toString("hex")}.sock`);
// Clean up any existing socket file
if (existsSync(socketPath)) {
unlinkSync(socketPath);
}
paths.push(socketPath);
sockets.push(
Bun.listen({
unix: socketPath,
socket: {
open(socket) {},
data(socket, data) {},
close(socket) {},
},
}),
);
// Verify the socket file was created
expect(existsSync(socketPath)).toBe(true);
}
// Stop all listeners
for (const listener of sockets) {
listener.stop();
}
// Verify all socket files were cleaned up
for (const path of paths) {
expect(existsSync(path)).toBe(false);
}
});
test.skipIf(isWindows)("Unix socket cleanup with active connections", async () => {
const socketPath = join(tmpdir(), `bun_test_active_${randomBytes(8).toString("hex")}.sock`);
// Clean up any existing socket file
if (existsSync(socketPath)) {
unlinkSync(socketPath);
}
let serverSocket = null;
let connectionReceived = false;
// Create a Unix socket listener
const listener = Bun.listen({
unix: socketPath,
socket: {
open(socket) {
serverSocket = socket;
connectionReceived = true;
},
data(socket, data) {
socket.write(data);
},
close(socket) {},
},
});
// Verify the socket file was created
expect(existsSync(socketPath)).toBe(true);
// Connect to the socket
const client = await Bun.connect({
unix: socketPath,
socket: {
open(socket) {},
data(socket, data) {},
close(socket) {},
},
});
// Wait for connection to be established
await Bun.sleep(10);
expect(connectionReceived).toBe(true);
// Stop the listener with force=true (should close all connections)
listener.stop(true);
// Give some time for cleanup
await Bun.sleep(10);
// Verify the socket file was cleaned up even with active connections
expect(existsSync(socketPath)).toBe(false);
});