fix(dgram): enable SO_REUSEPORT on macOS and improve error reporting#27772
fix(dgram): enable SO_REUSEPORT on macOS and improve error reporting#27772
Conversation
…27771) `bsd_set_reuseport()` was restricted to Linux only, causing `reusePort: true` in `node:dgram` to fail on macOS with a generic "Failed to bind socket" error, even though macOS natively supports SO_REUSEPORT. Now enabled on all platforms that define SO_REUSEPORT. Also fixed missing errno propagation when `bsd_set_reuse()` fails in UDP socket creation, and a socket fd leak in that error path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
WalkthroughEnable SO_REUSEPORT where available (not Linux-only), record reuse-related socket errors into the provided err (WSAGetLastError() on Windows, errno elsewhere), update enum comment about platform behavior, and add regression tests for UDP reusePort binding. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-usockets/src/bsd.c (1)
1246-1253: 🧹 Nitpick | 🔵 TrivialPre-existing issue: Socket leak and missing error on IPV6_V6ONLY failure.
This error path returns
LIBUS_SOCKET_ERRORwithout closing the socket or setting*err, which is inconsistent with the new error handling pattern added above. This would leak the file descriptor.Consider applying consistent error handling:
♻️ Suggested fix for consistency
`#ifdef` IPV6_V6ONLY if (listenAddr->ai_family == AF_INET6) { int enabled = (options & LIBUS_SOCKET_IPV6_ONLY) != 0; if (setsockopt(listenFd, IPPROTO_IPV6, IPV6_V6ONLY, &enabled, sizeof(enabled)) != 0) { + if (err != NULL) { +#ifdef _WIN32 + *err = WSAGetLastError(); +#else + *err = errno; +#endif + } + bsd_close_socket(listenFd); + freeaddrinfo(result); return LIBUS_SOCKET_ERROR; } } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bun-usockets/src/bsd.c` around lines 1246 - 1253, The IPV6_V6ONLY error path leaks listenFd and doesn't set *err; update the failure branch inside the IPV6_V6ONLY block (the setsockopt call that currently returns LIBUS_SOCKET_ERROR) to close(listenFd), set *err to the appropriate errno (or existing error variable used by this file), and then return LIBUS_SOCKET_ERROR so it matches the error-handling pattern used above; reference the symbols listenFd, listenAddr, options, LIBUS_SOCKET_IPV6_ONLY, and the setsockopt call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27771.test.ts`:
- Around line 33-43: The duplicated socket-bind Promise should be replaced by
extending the existing bindSocket helper to accept an optional port parameter;
modify bindSocket (the helper that currently takes a boolean reusePort) to add a
second optional parameter port?: number and use that port when calling
socket.bind, preserving the current default behavior when port is undefined,
then update the test to call bindSocket(true, port) for the second socket
instead of inlining the Promise so both bindings reuse the same helper (ensure
references to createSocket, reuseAddr/reusePort options and the returned { ok,
error, socket } shape remain unchanged).
---
Outside diff comments:
In `@packages/bun-usockets/src/bsd.c`:
- Around line 1246-1253: The IPV6_V6ONLY error path leaks listenFd and doesn't
set *err; update the failure branch inside the IPV6_V6ONLY block (the setsockopt
call that currently returns LIBUS_SOCKET_ERROR) to close(listenFd), set *err to
the appropriate errno (or existing error variable used by this file), and then
return LIBUS_SOCKET_ERROR so it matches the error-handling pattern used above;
reference the symbols listenFd, listenAddr, options, LIBUS_SOCKET_IPV6_ONLY, and
the setsockopt call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/bun-usockets/src/bsd.cpackages/bun-usockets/src/libusockets.htest/regression/issue/27771.test.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add optional port parameter to bindSocket() helper instead of inlining a duplicate Promise in the two-socket test case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The production changes look correct and well-structured, but since this is AI-generated C code in a low-level networking library, it warrants a quick human look -- particularly the SO_REUSEPORT platform guard change and the new error/cleanup path in bsd_create_udp_socket().
Extended reasoning...
Overview
This PR makes three changes to packages/bun-usockets/src/bsd.c and updates a comment in libusockets.h:
- Enables SO_REUSEPORT on macOS/BSDs by changing the preprocessor guard from #if defined(linux) to #if defined(SO_REUSEPORT) && !defined(_WIN32)
- Fixes a socket fd leak in bsd_create_udp_socket() when bsd_set_reuse() fails
- Fixes missing errno propagation in that same failure path
- Adds a new regression test for reusePort: true in dgram sockets
Security risks
No direct security risks. The changes improve resource cleanup (fixing an fd leak) and error reporting. The SO_REUSEPORT enablement on macOS does not introduce new attack surface -- it enables an OS-level feature that was already available on Linux.
Level of scrutiny
This is production-critical C code in the low-level socket layer (bun-usockets). While the changes are small and follow existing patterns in the codebase (the new cleanup block mirrors the bind() failure path 40 lines below), the PR is AI-generated (claude label) and has received no human review yet. C networking code deserves human verification even for straightforward changes, given the potential for subtle resource leaks or platform-specific behavior differences.
Other factors
- The new cleanup block (errno propagation + bsd_close_socket + freeaddrinfo) exactly matches the pattern at the bind() failure path (lines 1285-1296), confirming correctness.
- The SO_REUSEPORT behavioral difference between Linux (kernel-level load balancing) and macOS (port sharing without load balancing) is correctly documented in the updated comment.
- Bug 1 (test nit): The new test lacks a Windows platform guard -- reusePort: true will fail on Windows since SO_REUSEPORT is unsupported there and LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE prevents silent fallback.
- Bug 2 (pre-existing): The adjacent IPV6_V6ONLY failure path at line 1250 has the same fd/addrinfo leak pattern this PR fixes, but was not addressed. This is pre-existing and not a blocker for this PR.
| test("dgram reusePort: single socket binds successfully", async () => { | ||
| const result = await bindSocket(true); | ||
| expect(result.ok).toBe(true); | ||
| expect(result.error).toBeNull(); | ||
| await new Promise<void>(r => result.socket.close(() => r())); | ||
| }); |
There was a problem hiding this comment.
🟡 Test test/regression/issue/27771.test.ts unconditionally expects reusePort: true to succeed, but Windows does not support SO_REUSEPORT. Since dgram.ts always sets LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE, the error won't be silently swallowed on Windows, causing both test cases to fail. Add test.skipIf(process.platform === "win32") or a runtime capability check similar to checkSupportReusePort() used by the existing test-dgram-reuseport.js.
Extended reasoning...
What the bug is
The new regression test at test/regression/issue/27771.test.ts tests reusePort: true functionality by calling bindSocket(true) and asserting expect(result.ok).toBe(true). However, Windows does not define SO_REUSEPORT, so these assertions will always fail on Windows.
The specific code path
When reusePort: true is passed to createSocket() in node:dgram, the JS layer in dgram.ts (line 302) always sets LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE in the options flags. This flows down to bsd_set_reuse() in bsd.c, which calls bsd_set_reuseport(). On Windows, bsd_set_reuseport() sets errno = ENOTSUP and returns -1. Back in bsd_set_reuse() (line 939-940), when errno == ENOTSUP is true, it checks whether LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE == 0 — but since that flag IS set, the condition is false, so the error propagates rather than being silently swallowed. The socket creation fails, the error event fires, and expect(result.ok).toBe(true) fails.
Why existing code doesn't prevent it
The test has no platform guard whatsoever. It runs unconditionally on all platforms. The existing Node.js compat test test/js/node/test/parallel/test-dgram-reuseport.js correctly handles this by using checkSupportReusePort() from test/js/node/test/common/udp.js, which probes whether the platform actually supports reusePort and skips the test if it does not.
Step-by-step proof on Windows
- Test calls
bindSocket(true)which creates a socket withreusePort: true dgram.tssetsLIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILUREflag (always)bsd_create_udp_socket()callsbsd_set_reuse(listenFd, options)bsd_set_reuse()callsbsd_set_reuseport(listenFd)sinceLIBUS_LISTEN_REUSE_PORTis setbsd_set_reuseport()on Windows hits the#elsebranch: setsWSASetLastError(WSAEOPNOTSUPP),errno = ENOTSUP, returns-1- Back in
bsd_set_reuse():errno == ENOTSUPis true, but(options & LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE) == 0is false (flag is set), so it falls through and returns the error bsd_create_udp_socket()stores errno into*err, closes the socket, and returnsLIBUS_SOCKET_ERROR- The error event fires on the JS socket,
bindSocket()resolves with{ ok: false, ... } expect(result.ok).toBe(true)fails — both test cases fail on Windows CI
Impact
This is a test-only issue — no production code is affected. However, it will cause Windows CI failures for both test cases in this file. The fix is straightforward: add test.skipIf(process.platform === "win32") to both test cases, or add a runtime capability check like the existing reusePort tests do.
| if (bsd_set_reuse(listenFd, options) != 0) { | ||
| if (err != NULL) { | ||
| #ifdef _WIN32 | ||
| *err = WSAGetLastError(); | ||
| #else | ||
| *err = errno; | ||
| #endif | ||
| } | ||
| bsd_close_socket(listenFd); | ||
| freeaddrinfo(result); | ||
| return LIBUS_SOCKET_ERROR; | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing bug: The IPV6_V6ONLY setsockopt failure path at line 1249-1250 has the same triple-bug pattern (fd leak, addrinfo leak, missing errno propagation) that this PR fixes in the bsd_set_reuse() path just 6 lines above. The bare return LIBUS_SOCKET_ERROR should be replaced with the same cleanup pattern used at lines 1233-1244 and at the bind() failure path at lines 1285-1296.
Extended reasoning...
What the bug is
In bsd_create_udp_socket(), when the setsockopt(listenFd, IPPROTO_IPV6, IPV6_V6ONLY, ...) call fails at line 1249, the function returns LIBUS_SOCKET_ERROR immediately without performing any cleanup. This means: (1) listenFd is leaked because bsd_close_socket() is never called, (2) the result addrinfo linked list is leaked because freeaddrinfo() is never called, and (3) *err is not set with the errno, so the caller gets no useful error information.
The specific code path
This occurs in bsd_create_udp_socket() at lines 1246-1253. When getaddrinfo() resolves to an IPv6 address (AF_INET6), the function creates a socket, successfully calls bsd_set_reuse(), then attempts to set IPV6_V6ONLY. If the setsockopt call fails (returns non-zero), the code at line 1250 does a bare return LIBUS_SOCKET_ERROR with no cleanup.
Why existing code doesn't prevent it
The PR correctly fixed the identical pattern in the bsd_set_reuse() failure path at lines 1233-1244, adding bsd_close_socket(listenFd), freeaddrinfo(result), and *err = errno. The bind() failure path at lines 1285-1296 also correctly does all three cleanup steps. However, the IPV6_V6ONLY path between these two was missed.
Step-by-step proof
- Call
bsd_create_udp_socket("::1", 5000, LIBUS_SOCKET_IPV6_ONLY, &err)on a system whereIPV6_V6ONLYsetsockopt could fail. getaddrinforesolves to anAF_INET6address, solistenAddr->ai_family == AF_INET6is true.bsd_create_socket()succeeds, allocating a file descriptor (e.g., fd=5).resultholds the addrinfo allocation.bsd_set_reuse()succeeds (returns 0), so we proceed past the now-fixed cleanup block.- At line 1249,
setsockopt(listenFd, IPPROTO_IPV6, IPV6_V6ONLY, ...)fails and returns non-zero. - Line 1250 executes:
return LIBUS_SOCKET_ERROR;— fd 5 is leaked,resultis leaked,errstill holds 0. - The caller has no way to close fd 5 or free
result. The error code in*erris 0, giving no diagnostic information.
Impact
While the IPV6_V6ONLY setsockopt failure is uncommon in practice, when it does occur it causes a file descriptor leak and a memory leak per call. On a long-running server process, repeated failures could exhaust file descriptors. The missing errno propagation also makes debugging harder since the Zig layer would fall through to a generic error message.
How to fix
Apply the same cleanup pattern used in the bsd_set_reuse() and bind() failure paths. Replace the bare return LIBUS_SOCKET_ERROR at line 1250 with:
if (err != NULL) {
#ifdef _WIN32
*err = WSAGetLastError();
#else
*err = errno;
#endif
}
bsd_close_socket(listenFd);
freeaddrinfo(result);
return LIBUS_SOCKET_ERROR;
2 participants
Summary
Fixes #27771.
SO_REUSEPORTon macOS (and other BSDs):bsd_set_reuseport()was restricted to#if defined(__linux__)only, causingreusePort: trueinnode:dgramto always fail on macOS with ENOTSUP. macOS has supportedSO_REUSEPORTsince at least 10.12 (Sierra), and the reporter confirmed it works with raw C sockets. Changed the guard to#if defined(SO_REUSEPORT) && !defined(_WIN32)to enable it on all platforms that define the constant.bsd_set_reuse()failed inbsd_create_udp_socket(),errnowas not stored into*err, causing the Zig layer to fall through to a generic "Failed to bind socket" error instead of a descriptive one (e.g., "bind ENOTSUP 0.0.0.0:56780"). Now properly propagates the errno.bsd_set_reuse()path inbsd_create_udp_socket()was not callingbsd_close_socket()before returning, leaking the file descriptor.Test plan
test-dgram-reuseport.jspasses on Linuxnode-dgram.test.jshas no new failurestest/regression/issue/27771.test.tspassesreusePort: falseandreusePort: true🤖 Generated with Claude Code