Conversation
A worker names a descriptor of the primary with listen({ fd }) or
bind({ fd }). The primary accepted a stream socket for a datagram query
and a datagram socket for a stream query. The worker then failed to use
its copy, gave the handle back, and the primary closed its own
descriptor. For the descriptor of a listening server of the primary,
that stopped the server.
clusterValidateFd now takes the kind of the query. A descriptor of
another kind, a datagram socket that is not AF_INET or AF_INET6, and a
number that is not an integer get EINVAL from the primary, as in node
v26.3.0. The primary leaves the descriptor open.
|
Status: ready for review, CI is green. The base is How to reproduce on main The parent passes a listening TCP socket to the primary as descriptor 3. The primary forks one worker under
The fixture is Two decisions that need a maintainer (details in the Notes of the PR body)
Merge order. #44172 alone lets two more call orders reach this bug ( CI. Build 122733 on 8810596: 181 of 181 jobs pass. Tests on the merged branch (8810596), debug build: Review. Each finding of the review has an answer in its thread. One thread is open: a worker that names the descriptor of a listening server of the primary. It is under "Not covered" in the PR body. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…eger Under SCHED_RR the listener of the primary refuses fd 3.5, and the worker gets bind EINVAL as in node. The row passes without the change of this PR.
A handle of the primary closes the descriptor it holds when its last
worker leaves. queryServer made a second handle for a descriptor that a
live handle held: for a second listen({ fd }) of one worker, and for a
query with another key for the same number (udp4 and udp6). The second
handle closed the descriptor under the first one, and the first one
closed the number again later.
queryServer now looks for the holder of the number before it makes a
handle: a SharedHandle, a RoundRobinHandle, or a dgram socket of the
primary itself. A held number gets EEXIST, the answer of node v26.3.0
under SCHED_RR. The kind of the descriptor is checked first, so a wrong
kind still gets EINVAL.
The lookup runs below the Windows dgram branch, so it needs no platform condition. It uses the private forEach of the Map.
2e3e0a1 to
f7522d2
Compare
|
Updated 12:24 AM PT - Oct 2nd, 2026
✅ @robobun, your commit 881059602284a943e243fad58157c3108f463178 passed in 🧪 To try this PR locally: bunx bun-pr 44185That installs a local version of the PR into your bun-44185 --bun |
There was a problem hiding this comment.
I reviewed the two new commits and found no bugs; the push addresses the earlier inline points (the holder lookup now goes through handles.$forEach, and the win32 gate is gone so the lookup runs on every platform). Because this deliberately diverges from node v26.3.0 in three call orders (udp4 then udp6, a port listen followed by naming the created socket, and one server listening twice before the first ack now get EEXIST where node serves), a maintainer should still decide whether that divergence and the EEXIST-vs-EADDRINUSE errno question are the behavior Bun wants.
What was reviewed:
- Both lazily-bound Rust functions exist with the stated arity (
cluster_validate_fdinsrc/runtime/node/node_cluster_binding.rs,js_dgram_is_fd_adoptedinsrc/runtime/socket/udp_socket.rs), and neither can throw or return a non-number on a numeric fd. - The
fdgetters onSharedHandle/RoundRobinHandleread plain fields and the native Listenerfdgetter, which returns a number on every platform, so thehandle.fd === fdcomparison cannot throw. - The check sits only on the
handle === undefinedpath after the ENOTSUP guard, and the refusal reply carriesdataconsistent with the other error replies inqueryServer. - The 12 new test rows assert exact
{code, syscall, errno}and cover both scheduling policies; the fd-scan forfd: "created"is bounded and fails loudly if it does not find exactly one new socket.
Extended reasoning...
The diff adds a holder lookup in the node:cluster primary (src/js/internal/cluster/primary.ts) that refuses a worker's { fd } query with UV_EEXIST when a SharedHandle, RoundRobinHandle, or a primary dgram socket already holds that descriptor, plus fd accessors on the two handle classes and 12 new rows in test/js/node/cluster.test.ts. It touches no auth, crypto, or injection surface; the only descriptor-level work is a read-only getsockopt/getpeername/getsockname in an existing Rust helper. The code is small and I found no bugs, and the second push resolved both of my earlier inline comments. Deferring rather than approving because the PR intentionally changes observable node:cluster behavior away from node in three call orders and the author explicitly leaves the errno choice (EEXIST vs. the EADDRINUSE proposed in nodejs/node#65020) and the ownership rule to a maintainer, which is a compat design decision rather than a mechanical change.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
…ter-one-handle-per-fd
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. WalkthroughCluster descriptor validation now checks socket type, descriptor ownership, and datagram address family before creating cluster handles. Descriptor accessors and tests cover stream and datagram operations under both cluster scheduling policies. ChangesCluster file descriptor validation
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change tightens cluster descriptor validation and ownership detection, and the new tests exercise it under both scheduling policies. No concrete merge-blocking issue was established. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
… and the disconnect after it The order udp4, net, udp4 on one datagram socket reaches the second udp4 ask with the descriptor open. A worker with two handles under one key does not leave on disconnect.
| function errnoOfFdQuery(message) { | ||
| const fd = message.fd; | ||
| const isUdp = message.addressType === "udp4" || message.addressType === "udp6"; | ||
| let held = isUdp && (isFdOfDgramSocket ??= $newRustFunction("udp_socket.rs", "jsDgramIsFdAdopted", 1))(fd); | ||
| if (!held) { | ||
| handles.$forEach(handle => { | ||
| if (handle.fd === fd) held = true; | ||
| }); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a primary that itself listens on an inherited descriptor and whose worker names the same number still loses that listener and closes the number twice, exactly as on the base branch. The holder lookup in errnoOfFdQuery at primary.ts:246-253 only sees the dgram registry and the entries of handles; a net.Server or Bun.listen listener of the primary is not a holder it can see, so a SharedHandle adopts the descriptor and closes it when the worker leaves. Fix: treat a descriptor that a live listener of the primary holds as held too, returning EEXIST like the other holders; the PR description lists this under "Not covered".
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: in the primary, net.createServer().listen({ fd: 3 }), then a worker calls server.listen({ fd: 3 }) under SCHED_NONE (or tls under SCHED_RR). queryServer reaches errnoOfFdQuery at src/js/internal/cluster/primary.ts:246; handles.$forEach at primary.ts:251-253 finds no handle because the primary's own net.Server is not in handles. errnoOfFdQuery returns 0, and primary.ts:325 constructs a SharedHandle; SharedHandle.ts:22 validateFd(3, false) passes and SharedHandle.ts:26 records { fd: 3 }. When the worker closes its server, close() at primary.ts:376-383 calls remove(), and SharedHandle.ts:67 closeRawHandle(3) closes the descriptor that the primary's own listener still polls; the primary's server silently stops accepting, and its later server.close() closes number 3 a second time. The base branch behaves the same; the diff adds the holder lookup but it cannot see this holder. No safeguard covers it: validateFd checks kind and connectedness only.
Verification: Trigger: the primary does net.createServer().listen({ fd: N }) and a worker calls listen({ fd: N }) under SCHED_NONE. That listener is never inserted into handles, so the holder lookup in errnoOfFdQuery (src/js/internal/cluster/primary.ts:249-253) finds nothing and returns 0. SharedHandle.remove (SharedHandle.ts:67) calls closeRawHandle(fd), closing the descriptor under the primary's live listener.
There was a problem hiding this comment.
This case is real. It is the first entry under "Not covered" in the PR body, and I left it out of this PR on purpose.
I measured it with release builds. The primary listens on descriptor 3 with its own net.Server. One worker under SCHED_NONE calls listen({ fd: 3 }) and closes its server when it listens. Then a client connects.
| runtime | answer to the worker | after that |
|---|---|---|
| node v26.3.0 | bind EEXIST |
descriptor 3 is open, the primary serves the client |
| bun, main | listening |
descriptor 3 of the primary is EBADF, the client gets no answer |
| bun, this PR | listening |
the same as main |
Node has this answer from libuv: uv_tcp_open returns UV_EEXIST for a descriptor that its loop watches already (tcp.c). The cluster code of Bun cannot see a listener that it did not make. A walk over the listen sockets of the loop can see it, but that is native code in packages/bun-usockets. This PR changes three builtin JS files and no native code, so that lookup is a change of its own.
Without the fix the worker has two handles under one key and never leaves on disconnect. The primary kills it, so the test fails on the answers.
|
The base of this PR is now
The PR body has the measurements. |
This branch has not been deployed
1 participant
Base: main. Until #44172 merges, this diff contains it (Notes). Merge before #44015.
Problem
listen({ fd: 3 })answerslistening. The primary closes its own descriptor 3 under the first server, and the worker never leaves on disconnect. A debug build printsclose(3[BADF]) = EBADF. This is an indication of a file descriptor UAF.queryServer(src/js/internal/cluster/primary.ts:294) makes a second handle for a descriptor that a live handle holds. That handle closes it when its worker leaves.udp4,net,udp4(Notes).Fix
queryServerlooks for the holder of the number before it makes a handle. A held number getsEEXIST, as in node v26.3.0 underSCHED_RR.test/js/node/cluster.test.ts(13 of 15 new tests fail on node:cluster: refuse a descriptor of the wrong kind in the primary #44172 alone), 97 vendored cluster tests.Background
SharedHandle, aRoundRobinHandle, or a dgram socket of the primary.EEXISTfor a repeated key only, which missesudp4thenudp6. A native owner table, which adds two locks to each port listen.Downsides
EEXIST. cluster: report EADDRINUSE when a worker listens twice on the same port nodejs/node#65020 proposesEADDRINUSE. A maintainer must decide both (Notes).{ fd }query reads each live handle once. Text ofbun: +881 B.Notes
The base and the two parts of the diff.
node_cluster_binding.rs, 4 lines ofSharedHandle.ts, and its tests. The part of this PR is +38 -2 insrc/js/internal/cluster/(primary.ts,SharedHandle.ts,RoundRobinHandle.ts) and +201 -13 in the test file.The worker that never leaves. One worker under
SCHED_NONEasks for one descriptor of the primary in the given order and closes nothing. Then the primary callsworker.disconnect(). All four columns are release builds. "main" is 367d939. The next two are c0924e1 without and with the part of this PR. "Never leaves" is no exit in 12 s.listeningtwo times, never leavesbind EEXIST, leaves with code 0ERR_INTERNAL_ASSERTIONlisteningtwo times, never leavesopen EEXIST, leaves with code 0listen EINVAL,open EINVAL, leavesbind EINVAL,listening, never leavesbind EINVAL,open EEXIST, leaves with code 0bind EINVAL, worker diesopen EINVAL,bind EINVAL, leavesopen EINVAL,listening, never leavesopen EINVAL,bind EEXIST, leaves with code 0open EINVAL, worker diesshared()insrc/js/internal/cluster/child.ts), and a disconnect closes the handles of that table. The second handle under a key replaces the first, so the first stays open and keeps the worker alive. A debug build stops before that, at$assert(handles.has(key) === false).Two questions for a maintainer.
EEXIST. Node v26.3.0 givesEEXISTunderSCHED_RR, fromlistenin its primary. cluster: report EADDRINUSE when a worker listens twice on the same port nodejs/node#65020 is open and answersEADDRINUSEwhen a worker asks again for a key that it holds. If Bun follows that PR later, the 8 rows where one worker asks two times under one key change their errno.leftmeans in every row.Where this comes from. Nobody reported it. A review of #44015 found it by reading. #31829 lists under known limitations: "
server.listen({ fd })on an fd that is already listening resolves rather than failing withEEXIST".Affected on main (Linux, measured with the fixture of the test):
SCHED_NONE: each order in the table.SCHED_RR, the default:tlsservers, dgram sockets, and two kinds of server on one descriptor. A second plainnetlisten getsEEXISTalready, because epoll refuses the second listener of the primary.The rows. One descriptor of the primary, asks in order. I ran each row on node v26.3.0 and on main with the fixture of the test, one row in one process. "main" is the release build of 367d939, whose cluster code equals main except for type annotations.
SCHED_NONElistening, descriptor closed, second worker getsENOBUFSERR_INTERNAL_ASSERTIONbind EEXIST-17, then the second worker listensSCHED_NONElistening, descriptor closedERR_INTERNAL_ASSERTIONopen EEXIST-17SCHED_NONElisteninglisteningopen EEXIST-17SCHED_NONElisteninglisteningbind EEXIST-17SCHED_NONElisten({ fd })two times before the first answerlisteninglisteningbind EEXIST-17SCHED_NONElistening, descriptor closed under the socket of the primaryopen EEXIST-17open EEXIST-17SCHED_NONElisten EINVAL22, descriptor closed, thenopen EINVALbind EINVAL-22, then the worker diesbind EINVAL-22, thenopen EEXIST-17SCHED_RRbind EEXIST-17bind EEXIST-17bind EEXIST-17SCHED_RRlistening, descriptor closedbind EEXIST-17bind EEXIST-17SCHED_RRlistening, descriptor closed, client gets no answerTypeErrorintls.Server._setServerData(null)bind EEXIST-17, client servedSCHED_RRlistening, descriptor closedbind EEXIST-17bind EEXIST-17SCHED_RRbind EINVAL-22bind EINVAL-22bind EINVAL-22SCHED_RRlisteningbind EEXIST-17bind EEXIST-17The three orders that node serves and this PR refuses. They are rows 3, 4 and 5. In rows 3 and 4, main and node v26.3.0 both answer
listening, and both then close the number two times. I measured the result of eachclose()of the primary on that number with gdb:0, then-9(EBADF), on main and on node. With one ask the result is0only. The second close is harmless unless another file took the number in between. So in these rows the visible change is an error where main and node serve. Row 5 is one server that callslisten({ fd })two times in one tick. UnderSCHED_RRboth runtimes refuse it already.Self-review. 38 concerns.
{ fd }query.handles.$forEach. TheMaphas its key type now, because the$methods do not resolve onMap<any, any>.bunHinton the new answer. The message staysbind EEXIST, the message of node.Not covered.
SCHED_NONEa handle still adopts it. When the worker closes its server, the primary closes the descriptor under its own server, and a client gets no answer (measured on main and on this PR). Node answersbind EEXIST, because libuv refuses a descriptor that its loop watches already (uv_tcp_open). The cluster code cannot see that holder. A walk over the listen sockets of the loop can, but that is native code inpackages/bun-usockets, and this PR has none.ENOTSUP. A held number getsEINVALthere and notEEXIST, becauseclusterValidateFdanswersEINVALfor every descriptor on Windows. By reading, all the numbers areSOCKEThandles there:Bun.listen({ fd })takes one, and thefdof a listener and ofclusterRawBindgive one. I could not run Windows, and no test names a descriptor there.datafield of the new answer has no test. Nothing in Bun reads it.Costs.
queryServer. On a release build of the first version of this change, which had the same condition, that was 5 bytecode instructions, 8 for a dgram port bind, and no call. I did not measure it again on this code.{ fd }query that reaches this branch: oneforEachover the live handles. A dgram query also callsjsDgramIsFdAdopted, which takes one lock.wc -c):primary.js8601 to 9298 B (+697),SharedHandle.js1705 to 1788 B (+83),RoundRobinHandle.js5212 to 5313 B (+101). The other three cluster modules are byte-identical.GeneratedJS2Native.h): 117 to 117. This PR has no native change.bun(size, release builds of c0924e1 without and with the part of this PR): 80,705,669 to 80,706,550 B (+881). That is the builtin JS above. Main at the same base (f4d755a) is 80,705,363 B.Tests run.
cluster.test.tshas 0 fail and no retry on the 11 lanes that run it. Linux (7 lanes, one with ASAN) and macOS (2 lanes): 62 pass, 1 skip. Windows (2 lanes): 27 pass, 38 skip, because the tests of this PR are POSIX only.On a debug build of the merged branch in my environment (
--timeout 240000, because one debug process takes about 4 s to start on this machine):bun bd test test/js/node/cluster.test.ts: 63 pass.SCHED_RRrows "net, net" and "udp4, then net". The table says why.fdofRoundRobinHandle(2 rows), nofdofSharedHandle(7 rows), holder before kind (4 rows). I measured this on the first version of this PR.clusterorlisten-fdin their name exit 0.tsc --noEmit -p src/js/tsconfig.jsonpasses.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/cluster.test.ts