Fix IPv4 loopback range and IPv6 scope-id address comparison (#5486)#5679
Conversation
compareAddress now distinguishes IPv6 addresses that differ only by their sin6_scope_id (e.g. fe80::1%eth0 vs fe80::1%eth1), so a connection to one scoped interface is no longer reused for another. Loopback recognition now covers the whole 127.0.0.0/8 range instead of only 127.0.0.1, fixing misclassification of hosts such as 127.0.0.2. Addresses zeroc-ice#5486 (items 7, 8).
There was a problem hiding this comment.
Pull request overview
This PR addresses two transport-layer correctness issues in the C++ runtime: IPv6 address equality now accounts for sin6_scope_id (to prevent incorrect connection reuse across scoped interfaces), and IPv4 loopback detection now correctly recognizes the full 127.0.0.0/8 range. It also adds targeted unit assertions to keep these behaviors from regressing.
Changes:
- Update
IceInternal::compareAddressto compare IPv6sin6_scope_id. - Expand IPv4 loopback recognition from only
127.0.0.1to all of127.0.0.0/8. - Add deterministic unit assertions in
cpp/test/Ice/infofor both behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/Ice/Network.cpp | Fixes IPv6 scoped address comparison and expands IPv4 loopback classification logic. |
| cpp/test/Ice/info/AllTests.cpp | Adds unit assertions covering the updated loopback and scope-id comparison behaviors. |
| // Distinguish scoped link-local addresses such as fe80::1%eth0 and fe80::1%eth1. | ||
| if (addr1.saIn6.sin6_scope_id < addr2.saIn6.sin6_scope_id) | ||
| { |
| // It's an IPv4 address. The whole 127.0.0.0/8 range is loopback, not just 127.0.0.1. | ||
| return (ntohl(addr.s_addr) & 0xFF000000u) == 0x7F000000u || IN_MULTICAST(ntohl(addr.s_addr)); |
| } | ||
|
|
||
| // Distinguish scoped link-local addresses such as fe80::1%eth0 and fe80::1%eth1. | ||
| if (addr1.saIn6.sin6_scope_id < addr2.saIn6.sin6_scope_id) |
There was a problem hiding this comment.
Is it safe to check this for non link-local addresses?
…dress) The new compareAddress test called htons and inet_pton directly; those are Winsock symbols (ws2_32.lib) and the info test client does not link Winsock on Windows, causing LNK2019/LNK1120. Build the address with the exported IceInternal::getAddressForServer helper (in the Ice DLL, which links Winsock) instead, matching how Ice tests avoid raw socket APIs.
Review feedback: compute ntohl(addr.s_addr) once instead of twice in the IPv4 loopback/multicast check.
pepone
force-pushed
the
fix/5486-network-addr
branch
from
fca8dcb to
ec0b057
Compare
bernardnormier
left a comment
bernardnormier
left a comment
There was a problem hiding this comment.
Both fixes look correct. Two small things:
- The test comments carry
(issue #5486 item N)references — we prefer tests to stand on their own without issue-number tags (the substantive explanation is great as-is). Suggestions below to drop them. - No changelog fragment needed here: binding to a
127.xaddress other than127.0.0.1, or to a zoned IPv6 link-local, is uncommon enough that this doesn't warrant a user-facing entry.
FYI I checked the other mappings for the same two bugs: C# and Java both classify the full 127.0.0.0/8 as loopback via the standard library (IPAddress.IsLoopback / InetAddress.isLoopbackAddress()), and their connection-reuse equality compares the IPv6 scope id (IPAddress.Equals / Inet6Address.equals). JS compares endpoints by host string, so neither bug applies. So this C++ fix is the only one needed (one minor Java compareAddress ordering nit aside).
Co-authored-by: Bernard Normier <bernard@zeroc.com>
bernardnormier
left a comment
bernardnormier
left a comment
There was a problem hiding this comment.
Approving. The C++ fixes are correct, and I confirmed C#/Java handle both cases via the standard library and JS compares by host string, so no parallel fixes are needed. Filed a follow-up issue for the minor Java compareAddress scope-id ordering nit. The test-comment suggestions above are optional cleanup.
Addresses #5486 (items 7, 8). One of a set of PRs splitting the grouped transport audit.
compareAddressnow comparessin6_scope_id, so two IPv6 addresses that differ only by their zone (e.g.fe80::1%eth0vsfe80::1%eth1) no longer compare equal — this previously allowed connection reuse to the wrong scoped interface.127.0.0.0/8range instead of only127.0.0.1, fixing misclassification of hosts such as127.0.0.2.Added deterministic unit assertions in
Ice/info, verified red→green with each item isolated. Codex-audited.