fix(start-os): reject a port range that overlaps a port the host binds - #3642
Open
helix-nine wants to merge 1 commit into
Open
fix(start-os): reject a port range that overlaps a port the host binds#3642helix-nine wants to merge 1 commit into
helix-nine wants to merge 1 commit into
Conversation
A host keeps single-port bindings and port ranges in two independent maps, and neither writer consulted the other, so a range covering an internal port the same host also bound singly (or covered by another range) was accepted silently. Only external ports were protected, by AvailablePorts. Two claims on one container socket are not merely redundant. A range's forward hardcodes secure: true and update_addresses applies no gateway-secure filtering to ranges, so an overlapping range serves a port on gateways that port's own BindOptions.secure excluded. It also leaves clear_bindings unable to tell the two apart: BindId carries only (host, internal port), so a range and a binding sharing a start port shield each other from being disabled. add_binding and add_binding_range now reject an overlapping claim before allocating anything, naming both spans. The check counts only enabled entries — clear_bindings disables every binding before a setup pass runs, and nothing prunes dormant ones, so counting them would permanently block moving existing single ports into a range. Computing the claimed span also bounds it server side for the first time; only the SDK checked it before. coturn-startos is the sole bindPortRange user in either org and its 3478 binding is disjoint from its 42000-42499 range, so no published package is affected and no migration is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Per @dr-bonez: a range overlapping a binding should be invalid.
A
Hostkeeps single-port bindings and port ranges in two independent maps(
bindingskeyed by internal port,binding_rangeskeyed by internal startport), and neither writer ever consulted the other. Only external ports were
protected, by the box-wide
AvailablePortsallocator — so a range covering aninternal port the same host also bound singly, or covered by another range, was
accepted silently.
Why it isn't merely redundant
ForwardRequirements.secure = true(
net_controller.rs:649-653— deliberate, ranges carry noSecurityoption),and
update_addressesapplies the gateway-secure filter to bindings but notto ranges. So an overlapping range serves that internal port on gateways the
binding's own
BindOptions.secureexcluded.BindIdcarries only(host, internal port)andclear_bindingstests oneexceptset against both maps (net_controller.rs:1235-1256), so a range anda binding sharing a start port shield each other from being disabled.
The change
Model<Host>::add_bindingandadd_binding_rangereject an overlapping claimbefore allocating anything, naming both spans. These are the only key-creating
writers to either map, so this also covers the OS's own
os_bindings.The check counts only
enabledentries.create_servicerunsclear_bindings(∅)before every init pass (net_controller.rs:220), so at bindtime "enabled" means "claimed by this generation"; nothing ever prunes dormant
entries, so counting them would permanently block moving existing single ports
into a range — the ZMQ 28332/28333 migration the packaging book itself
documents — failing on every existing install while passing on a fresh one.
Computing the claimed span also bounds it server-side for the first time:
internalStartPort + numberOfPortspast 65535 was checked only in the SDK(
Host.ts:212) and otherwise failed late inforward-port.Blast radius
coturn-startosis the onlybindPortRangeuser across all 123 non-archived*-startosrepos in both orgs, and its3478binding is disjoint from its42000-42499range. The OS's admin host binds a single port and can never holda range. The 0.3.x compat shim binds single ports only. No migration needed —
because the check reads only enabled entries, existing db state cannot make a
bind fail.
Tests
8 new:
internal_spanbounds, enabled-only occupancy, self-skip on idempotentre-bind (
net/host/binding.rs); and the wiring — range blocked by an enabledbinding with nothing allocated, dormant binding not blocking the range that
supersedes it, single bind blocked inside an enabled range, coturn's shape
accepted, overflow rejected (
net/host/mod.rs).cargo test -p start-core --features=test net::→ 208 passed.make start-core-format-checkclean.Reviewer calls
eyre!, matching all 11 errors innet/host/(zero uset!, including"domain {domain} is already in use",which is squarely user-facing).
start-core/AGENTS.md:39says error stringsmust be localized. I followed the module; say the word and I'll add the keys.
a range in host A covering host B's port has the same problem. Rejecting that
is a bigger policy call (two hosts binding the same single port looks like a
legitimate existing pattern), so it's left alone.
sibling and free its external ports, which would make the port-preserving
singles→range migration work (today
try_alloc_rangehard-errors when thedormant binding holds an external port inside the requested span). That's a
behavior change on its own; not included here.
Separate from #3639 — no code overlap, only a neighbouring
CHANGELOG.mdbullet.