fix(tokio): refuse a listen address an existing reuseport group holds - #2977
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38ddf951d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shard.is_some_and(|shard| shard.index() == 0) { | ||
| drop(crate::bind::udp(crate::bind::Udp::new(addr))?); | ||
| } |
There was a problem hiding this comment.
When two same-UID worker groups start concurrently on a free address, both can complete and drop this plain probe before either performs the following reuseport bind. Both reuseport binds then succeed and join the same group, rather than one receiving the claimed startup failure, so their filters can again direct traffic to only one process and later renumber live connections. The new regression test starts its groups serially and cannot exercise this window; preventing the silent failure requires an exclusivity mechanism held through group construction rather than a probe that is released before the actual bind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 22848ee with the group-lifetime lock this called for: an abstract-namespace unix socket named by the listen address, taken before the first member binds and held until the group is gone (abstract names share the reuseport group's network-namespace scope and die with the process). Concurrent constructions now exclude each other for the group's lifetime; the probe remains for holders the lock cannot see (pre-lock relays, unrelated same-UID processes). Both mechanisms are independently mutation-checked in the Linux tests.
(written by Fable 5)
38ddf95 to
22848ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22848eee32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| use std::os::linux::net::SocketAddrExt; | ||
|
|
||
| let name = format!("moq-quic-workers/{addr}"); |
There was a problem hiding this comment.
On a multi-user Linux host where port 443 is privileged, any unprivileged process sharing the relay's network namespace can bind this predictable abstract AF_UNIX name because abstract sockets have no filesystem permission checks. That process can then prevent a privileged relay from starting even though it cannot bind the UDP port or join its reuseport group; use a filesystem socket or lock file in a runtime directory writable only by the effective UID. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| // lone worker may use one, and its port cannot be named in advance. | ||
| let lock = match requested.port() { | ||
| 0 => None, | ||
| _ => Some(crate::steer::Lock::acquire(requested).map_err(|err| match err.kind() { |
There was a problem hiding this comment.
The fresh evidence after the earlier review is the newly added Lock::acquire path here: it is specifically intended to handle two constructors that pass the plain-bind probe simultaneously, but both added tests invoke Workers::bind serially. Removing or breaking the lifetime lock while retaining the probe would therefore leave the suite green and restore the interleaved reuseport-group failure, so add a synchronized concurrent-bind regression that fails without the lock. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L133-L133
Useful? React with 👍 / 👎.
22848ee to
e3eb307
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3eb307626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// the dual-stack group's IPv4 side. A specific address keeps its one name; | ||
| /// its overlap with a wildcard is left to the probe. |
There was a problem hiding this comment.
When one group binds 0.0.0.0:p (or [::]:p) while another concurrently binds a concrete address on the same port, names gives them disjoint locks and both plain probes can finish before either reuseport bind. Linux then accepts both binds, with the concrete socket taking traffic for that address and silently removing it from the wildcard relay even though both groups report ready. The probe cannot serialize this construction path, so coordinate wildcard and concrete lock names and add a synchronized regression for it. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L129-L133
Useful? React with 👍 / 👎.
| // Somebody holds the name; connecting tells us who. A holder that vanished | ||
| // between the bind and here reads as a real overlap, which a retry resolves; | ||
| // erring loud beats erring silent. | ||
| let uid = UnixStream::connect_addr(&unix).and_then(|stream| peer_uid(&stream))?; |
There was a problem hiding this comment.
On a multi-user Linux host, a foreign UID can bind the predictable abstract name as SOCK_DGRAM instead of a UnixListener; UnixListener::bind_addr still returns AddrInUse, but this stream-only connection returns ConnectionRefused or WrongProtocolType, which ? propagates and Workers::bind turns into a startup failure. The fresh evidence beyond the earlier squatting finding is this new UID mitigation's dependence on the holder accepting Unix stream connections, so another socket type still lets an unprivileged process block a privileged relay. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L102-L106
Useful? React with 👍 / 👎.
e3eb307 to
183c68e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 183c68e4b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(runtime) = std::env::var_os("XDG_RUNTIME_DIR").filter(|dir| !dir.is_empty()) { | ||
| let dir = std::path::PathBuf::from(runtime).join("moq"); | ||
| if std::fs::create_dir_all(&dir).is_ok() { | ||
| return Some(dir); |
There was a problem hiding this comment.
The fresh evidence beyond the earlier lock finding is that dir() selects its lock inode from each process's environment and filesystem. Two same-UID relays sharing a network namespace can use different lock paths when, for example, a system service has no XDG_RUNTIME_DIR while an interactive process does, or host-network containers have separate mount namespaces. Their flocks then do not conflict, so concurrent constructors can both pass the plain probe and silently join the same reuseport group, restoring the traffic-loss race this change is intended to fix; use an exclusion mechanism shared by the network namespace and add a regression covering distinct lock-directory contexts. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L129-L133
Useful? React with 👍 / 👎.
3298e09 to
4517d90
Compare
SO_REUSEPORT groups sockets by address and UID, so a worker group binding an address a same-UID process already serves joined that process's group instead of failing. Two relays overlapping in a rolling restart hit exactly that: the old group's cBPF filter covers the whole group and keeps steering every packet to the old members, so the new relay reports ready while serving nothing, and the old process exiting renumbers the survivors out from under their issued connection IDs. Without workers the same overlap is a loud EADDRINUSE. Two mechanisms close it, because neither is sufficient alone: - A group-lifetime lock: a flocked file keyed by the port, taken before the first member binds and released by the kernel with the descriptor, so it cannot go stale and dies with its process. This is what excludes two groups constructing concurrently, where a probe alone races: both could probe and release before either binds, and preemption makes that window unbounded. Port-only keying gives every address space that can overlap on a port ([::], 0.0.0.0, and any specific address) one lock, trading over-exclusion (distinct addresses sharing a port are refused although they could coexist, loudly, naming the port) for never racing across spellings. The file lives in a directory only this UID can write, created 0700 and verified by ownership, type, and mode whether it came from XDG_RUNTIME_DIR or the per-UID temp fallback (an environment variable is not proof), and is opened O_NOFOLLOW. That is what makes the lock immune to other users: a reuseport group only admits same-UID sockets, so same-UID is exactly the set to exclude, and no other user can touch the lock to deny startup. If no protected directory can be had, the group starts on probe-only protection with a warning rather than refusing. - A plain-bind probe by the first member, for holders the lock cannot see: a relay predating it, or an unrelated same-UID process. A plain bind refuses a port any socket holds, reuseport or not, so an already-bound foreign group is refused with the same AddrInUse as everywhere else. Each mechanism is mutation-checked on Linux, independently. Neutering the lock fails exactly the three lock tests: a second group on the same address, the other wildcard spelling, and a distinct address sharing the port (that run visibly formed the silent second group the lock exists to prevent). Removing the probe fails the foreign-group test. The occupied-address test also rebinds after shutdown, proving the lock dies with its group. All 11 worker integration tests, 207 moq-tokio lib tests, and the relay's runtime_workers test pass in a container on kernel 6.19 aarch64. Also resolved while here: the group resolves its listen address once up front (hostnames included), so the lock, the probe, and every member agree on the address a rotating DNS answer could otherwise split. Fixes #2960 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4517d90 to
1614acd
Compare
1 participant
Fixes #2960. Targets
devbecause the worker mode only exists there (#2921).Root cause
SO_REUSEPORTgroups sockets by(address, port)and effective UID, so a worker group binding an address a same-UID process already serves joins that process's group instead of failing. Two relays overlapping in a rolling restart hit exactly that:0..N-1with a cBPF filter reducing modulo N.N..2N-1and attach a filter of their own, but a reuseport filter covers the whole group, and both reduce modulo N, so every packet keeps selecting the old members.Without workers the same overlap is a loud
EADDRINUSEand the new process exits, which is the behavior this restores.Fix: a group-lifetime lock, plus a probe
Two mechanisms, because neither is sufficient alone:
A group-lifetime lock: an
flocked file keyed by the port, taken before the first member binds and released by the kernel with the descriptor, so it cannot go stale and dies with its process. This is what excludes two groups constructing concurrently: a probe alone races there, since both processes can probe and release before either binds, and preemption makes that window unbounded (caught in review by Codex). A second group fails with a dedicatedWorkerOverlaperror naming the port.Two design points, both from review rounds that broke earlier shapes of this lock (an abstract-namespace socket keyed by address):
[::]dual-stack,0.0.0.0, any specific address) shares one lock, so no two spellings can construct concurrently under different keys. The trade is over-exclusion: two same-UID groups on distinct addresses sharing a port are refused although they could coexist. That refusal is loud and names the port; the failure it prevents was silent traffic loss.XDG_RUNTIME_DIR, else a verified 0700 per-UID temp dir). A reuseport group only admits same-UID sockets, so same-UID is exactly the set the lock must exclude, and filesystem permissions mean no other user can squat or tamper with it to deny startup (the flaw of the abstract-namespace approach, which has no permission checks). If no protected directory can be had, the group starts on probe-only protection with a warning rather than refusing to start.A plain-bind probe by the first member, for holders the lock cannot see: a relay predating this fix, or an unrelated same-UID process. A plain bind refuses a port any socket holds, reuseport or not, so an already-bound foreign group is refused with the same
AddrInUseas everywhere else.The group also resolves its listen address once up front, so the lock, the probe, and every member agree on the address a rotating DNS answer could otherwise split. An ephemeral port takes no lock: only a lone worker may use one, and its port cannot be named in advance.
The eBPF
SK_REUSEPORT+SOCKARRAYselector from #2875 remains the structural fix (selection by map slot survives membership changes entirely); until then, the silent-failure case is closed rather than mitigated.Testing
Each mechanism is independently mutation-checked on Linux. Neutering the lock fails exactly the three lock tests (same address, the other wildcard spelling, a distinct address sharing the port; that run visibly formed the silent second group the lock exists to prevent), and removing the probe fails the foreign-group test:
an_occupied_port_is_refused: a second group on a served address fails withWorkerOverlap, and binds cleanly again after the first group shuts down (the lock dies with its group). With the lock neutered, this test fails.a_foreign_reuseport_group_is_refused: a reuseport socket bound outside any group (no lock held) is refused by the probe. With the probe removed, this test fails because the group silently joins the foreign member.New tests also cover the wildcard-spelling refusal and the deliberate distinct-address-same-port over-exclusion. All 11 worker integration tests, all 207
moq-tokiolib tests (steering end-to-end included), and the relay'sruntime_workersintegration test pass in a container on kernel 6.19 aarch64.just checkandjust test(1917 tests) pass on macOS.Cross-package sync
No wire change (
drafts/untouched), nomoq-ffisurface change, no CLI change.doc/bin/relay/config.mdneeds no update: the fix removes a hazard rather than adding behavior, restoring parity with the non-worker path.Found in passing, filed separately: #2979 (
moq-tokio --no-default-featuresdoes not compile ondev, pre-existing).🤖 Generated with Claude Code
(written by Fable 5)