Align CIDR containment and bounds with CIDRv4 and CIDRv6 - #827
Conversation
`CIDR` keeps the address exactly as parsed, so a block written from a host address keeps its host bits. `contains` compared that unmasked value against the masked probe, so the block reported that it excluded its own members. Mask both sides, as `CIDRv4` and `CIDRv6` already do. `lower` also dropped the IPv6 zone while `upper` kept it, which put a zoned block's lower bound outside the block because `contains` compares zones. The vmnet prefix helper rendered `lower` into `inet_pton`, which rejects a zone suffix, so it now renders the network address on its own.
| // `inet_pton` rejects a zone suffix, so render the network address on | ||
| // its own. A vmnet prefix is never link-scoped, so dropping the zone | ||
| // here loses nothing. | ||
| inet_pton(AF_INET6, IPv6Address(prefixV6.lower.value).description, &p) |
There was a problem hiding this comment.
| // `inet_pton` rejects a zone suffix, so render the network address on | |
| // its own. A vmnet prefix is never link-scoped, so dropping the zone | |
| // here loses nothing. | |
| inet_pton(AF_INET6, IPv6Address(prefixV6.lower.value).description, &p) | |
| // `inet_pton` rejects a zone suffix, so render the network address on | |
| // its own. A vmnet prefix is never link-scoped, so dropping the zone | |
| // here loses nothing. | |
| guard inet_pton(AF_INET6, IPv6Address(prefixV6.lower.value).description, &p) == 0 else { | |
| throw ContainerizationError(...) | |
| } |
I'd prefer we be strict here.
There was a problem hiding this comment.
Done in 9b9907d. configurePrefixV6 no longer strips the zone: it renders prefixV6.lower as-is and guards on the inet_pton result, so a zoned prefix is rejected instead of quietly coerced, and supplying a zoneless prefix is the caller's job.
One deviation from the suggestion — inet_pton returns 1 on success and 0 when the string doesn't parse, so the guard reads == 1. As written (== 0) it would have thrown on every valid prefix.
Checking the result also closes something older than this PR: it was discarded, so any address inet_pton refused left p zeroed and configured ::.
No call sites needed changing — every in-repo caller already passes a zoneless prefix (CIDRv6("fd00::/64") in the integration suite).
Per review, the vmnet path no longer strips the zone itself. It renders `lower` as-is and checks the `inet_pton` result, so a prefix it cannot parse is rejected instead of quietly accepted, leaving the caller responsible for supplying a zoneless prefix. Checking the result also closes a pre-existing hole: it was discarded, so an address `inet_pton` refused left the buffer zeroed and configured `::`.
|
@jglogan pushed 9b9907d — the vmnet helper now leaves the zoneless prefix to the caller and checks Re-verified locally: |
2 participants
Fixes #826.
What
CIDRkeeps the address exactly as parsed, so a block written from a host address keeps its host bits set.containscompared that unmasked stored value against the masked probe, so the wrapper reported that a block excluded its own members while the types it wraps answered the same query correctly.CIDRv4/CIDRv6192.168.1.100/24contains192.168.1.100falsetrue192.168.1.100/24contains192.168.1.0falsetrue10.1.2.3/16contains10.1.99.99falsetrue2001:db8::1234/64contains2001:db8::1falsetrueMasking both sides matches what
CIDRv4.containsandCIDRv6.containsalready do. A block written in network form was unaffected, which is why this went unnoticed: every containment case inTestCIDR.swiftused a network address.The same accessors carried a second defect.
lowerdropped the IPv6 zone whileupperkept it, and becausecontainscompares zones, a zoned block excluded its own lower bound:lowernow carries the address's zone the wayupperalready did, in both copies of the logic: theCIDRwrapper andCIDRv6.One caller needed a matching change.
VmnetNetwork.configurePrefixV6renderslowerintoinet_pton, which rejects a zone suffix, so it was relying onlowerstripping it. Per review, the vmnet API now leaves that to the caller: the helper rendersloweras-is and checks theinet_ptonresult, rejecting a prefix it cannot parse rather than quietly accepting a zoned one. That check also closes a hole older than this PR — the result was discarded, so any addressinet_ptonrefused left the buffer zeroed and configured::. Every in-repo caller already passes a zoneless prefix, so no call site changes.Verification
TestCIDR.swift: containment across v4 and v6 host-address blocks, non-members still excluded, the wrapper agreeing withCIDRv4/CIDRv6over a probe set, zoned bounds contained in their own block, zone rendering, and unzoned bounds still carrying no zone.CIDR.swiftandCIDRv6.swiftwhile keeping the tests fails 7 of the 9, with the wrong values visible in the output, for example(block → 2001:db8::5%lo0/126).contains(block.lower → 2001:db8::4)and(wrapper.contains(.v4(ip)) → false) == (concrete.contains(ip) → true). The 2 that still pass are the controls written to pass either way: non-members stay excluded, and unzoned bounds are untouched.make containerizationbuilds clean with the defaultWARNINGS_AS_ERRORS=true.swift format lint --strict --recursive --configuration .swift-format-nolint Sources Testsexits 0,swift formatleaves all four files unchanged, andmake check-licensesreports no missing headers.#if os(macOS), private, and reachable only throughVmnetNetwork.init, which creates a real vmnet network. It is exercised by the integration suite, which needs the vmnet entitlement.The public shape of
CIDR,CIDRv4andCIDRv6is untouched; only the values these accessors compute change.