From 4bae6edd761f2c8bb1a3b8a7a2b65dd23dd00caf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 30 Jul 2026 22:31:13 -0500 Subject: [PATCH 01/20] hotfix --- noq-proto/src/connection/mod.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index b12e7c2ab6..5195a76e76 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2339,7 +2339,15 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && known_path.network_path.local_ip != network_path.local_ip + && known_path + .network_path + .local_ip + .as_ref() + .map(std::net::IpAddr::to_canonical) + != network_path + .local_ip + .as_ref() + .map(std::net::IpAddr::to_canonical) && !local_ip_may_migrate { trace!( From a75a9bed29c2645a033ffe2623888cb7ce9e4612 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sat, 8 Aug 2026 00:50:30 -0500 Subject: [PATCH 02/20] fix(proto): canonicalize FourTuple comparisons, not storage (noq#738) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `4bae6edd` (the hotfix posted in #738) and #783 both patch individual `==`/`!=` comparison sites in `Connection::early_discard_packet` to canonicalize IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) before comparing, since dual-stack sockets can report the same peer in either form depending on the code path. This closes the same class of bug at every site where `FourTuple`'s `PartialEq`/`Hash` or a raw `remote` comparison is used, not just the two `early_discard_packet` sites. An earlier version of this branch canonicalized `remote`/`local_ip` inside `FourTuple::new()`, mutating the stored/emitted address so every downstream comparison would see one canonical form automatically. That had two real problems, both found by running the existing test suite: - It changes the address family of what gets handed to the OS for sending (`Transmit::destination` becomes plain IPv4 where it used to be IPv4-mapped IPv6 on dual-stack sockets). This crate's CI is Linux-only, so whether that's safe on Windows/macOS was unverified. It also broke the `noq` crate's `normalize_network_path` IPv6 autodetection, silently narrowing genuine mixed v4/v6 multipath. - It broke the test harness's own simulated network routing (21 proptest regressions), because `tests/util.rs` compares raw `SocketAddr`s captured before canonicalization against ones captured after. This version instead canonicalizes only for comparison/hashing, never for storage: `FourTuple::new()` is unchanged from `main`, and `PartialEq`/`Hash` are hand-written to canonicalize via a `remote_key()` helper — `(ip.to_canonical(), port, scope_id)`, keeping `scope_id` for addresses that stay IPv6 (a mapped address canonicalizes to plain IPv4 and has no scope). Dropping `scope_id` unconditionally was tried first and collapsed two genuinely different link-local interfaces into one path for equality/hashing — the same class of bug this fix is meant to prevent, just for a different field. Covered by a new regression test. `is_probably_same_path` and the other raw `remote == remote` comparisons that don't go through `FourTuple`'s whole-struct equality (`early_discard_packet`, PATH_CHALLENGE on-path detection, OBSERVED_ADDRESS matching, the peer-migration trigger, and `PathResponses::push`'s dedup) now all route through a shared `same_remote()` helper instead of open-coding the comparison. Includes and builds on the regression test from the `noq-738` branch (`noq-proto/src/tests/multipath.rs::open_path_with_explicit_local_ip`), adapted to build its `ManyToManyRouting` via `add_client_route`/ `add_server_route` instead of `from_routes` (which now rejects the duplicate `server_addr` this test intentionally uses, an invariant added by #721 after the test was originally written), and to construct its `FourTuple` via `FourTuple::new()` rather than a struct literal. Testing: - Two new unit tests in `noq-proto/src/lib.rs` (`four_tuple_tests`): `four_tuple_eq_ignores_mapped_v4_representation` (the #738 case) and `four_tuple_eq_preserves_link_local_scope_id` (regression test for the scope_id issue found during review). - `cargo test -p noq-proto`: 390 passed, 0 failed. - `cargo test -p noq --lib`: 32 passed, 3 ignored, 0 failed, including `echo_dualstack` (which the construction-time version of this fix broke). - Verified on a real Android device (WiFi + cellular): `Secondary` established, then `PhysicalWifi`/`PhysicalCellular` both validate on the first attempt instead of retrying 3x and getting abandoned. Co-authored-by: Philipp Krüger --- noq-proto/src/connection/mod.rs | 18 ++++-- noq-proto/src/connection/paths.rs | 6 +- noq-proto/src/lib.rs | 101 +++++++++++++++++++++++++++++- noq-proto/src/tests/multipath.rs | 73 ++++++++++++++++++++- 4 files changed, 187 insertions(+), 11 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 5195a76e76..9ec5bca22f 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -19,7 +19,7 @@ use tracing::{debug, error, trace, trace_span, warn}; use crate::{ Dir, Duration, EndpointConfig, FourTuple, Frame, INITIAL_MTU, Instant, MAX_CID_SIZE, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, Side, StreamId, TIMER_GRANULARITY, TokenStore, Transmit, - TransportError, TransportErrorCode, VarInt, + TransportError, TransportErrorCode, VarInt, same_remote, cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, config::{ServerConfig, TransportConfig}, @@ -2327,7 +2327,9 @@ impl Connection { // forbids migration, drop the datagram. This could be relaxed to heuristically // permit NAT-rebinding-like migration. if let Some(known_path) = self.path_mut(path_id) { - if network_path.remote != known_path.network_path.remote && !peer_may_probe { + // noq#738: canonicalize both sides, same rationale as the local_ip comparison + // below and `FourTuple`'s `PartialEq`. + if !same_remote(network_path.remote, known_path.network_path.remote) && !peer_may_probe { trace!( %path_id, %network_path, @@ -4993,7 +4995,8 @@ impl Connection { let path = &mut self .path_mut(path_id) .expect("payload is processed only after the path becomes known"); - if network_path.remote == path.network_path.remote { + // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. + if same_remote(network_path.remote, path.network_path.remote) { // PATH_CHALLENGE on active path, possible off-path packet // forwarding attack. Send a non-probing packet to recover the // active path. See @@ -5290,7 +5293,8 @@ impl Connection { let space_open_status = self.spaces[SpaceKind::Data].for_path(path_id).open_status; let path = self.path_data_mut(path_id); - if path.network_path.remote == network_path.remote { + // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. + if same_remote(path.network_path.remote, network_path.remote) { if let Some(updated) = path.update_observed_addr_report(observed) && space_open_status == OpenStatus::Informed { @@ -5571,10 +5575,11 @@ impl Connection { && let Some(new_local_ip) = network_path.local_ip { let path_data = self.path_data_mut(path_id); + // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. if path_data .network_path .local_ip - .is_some_and(|ip| ip != new_local_ip) + .is_some_and(|ip| ip.to_canonical() != new_local_ip.to_canonical()) { debug!( %path_id, @@ -5587,10 +5592,11 @@ impl Connection { } // If the peer migrated to a new address, trigger migration. + // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. if self.peer_may_migrate() && (migrate_on_any_packet || !is_probing_packet) && is_largest_received_pn - && network_path.remote != self.path_data(path_id).network_path.remote + && !same_remote(network_path.remote, self.path_data(path_id).network_path.remote) { self.migrate(path_id, now, network_path, migration_observed_addr); // Break linkability, if possible diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index efe71812f6..c6eab62a36 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -12,7 +12,7 @@ use super::{ }; use crate::{ ConnectionId, Duration, FourTuple, Instant, TIMER_GRANULARITY, TransportConfig, - TransportErrorCode, VarInt, + TransportErrorCode, VarInt, same_remote, coding::{self, Decodable, Encodable}, congestion, connection::{MAX_BACKOFF_EXPONENT, MAX_PTO_INTERVAL}, @@ -872,10 +872,12 @@ impl PathResponses { token, network_path, }; + // noq#738: canonicalize both sides before comparing, same rationale as + // `FourTuple`'s `PartialEq`. let existing = self .pending .iter_mut() - .find(|x| x.network_path.remote == network_path.remote); + .find(|x| same_remote(x.network_path.remote, network_path.remote)); if let Some(existing) = existing { // Update a queued response if existing.packet <= packet { diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 61f26e7ed9..2012a8a6b2 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -367,7 +367,18 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// paths exist with the same remote, but different local IP interfaces. /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. -#[derive(Hash, Eq, PartialEq, Copy, Clone)] +/// +/// noq#738: `PartialEq`/`Eq`/`Hash` are hand-written (see below) to canonicalize +/// IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) down to plain IPv4 *only for +/// comparison/hashing purposes* — the `remote`/`local_ip` fields themselves are left +/// exactly as constructed. Canonicalizing the stored value instead (i.e. in `new()`) +/// was tried and reverted: it changes the address family of what gets handed to the +/// OS for sending (`Transmit::destination`), which is unverified on non-Linux +/// platforms (this crate's CI is Linux-only) and broke IPv6-vs-IPv4 autodetection in +/// the `noq` crate's `normalize_network_path`. Comparing canonically without mutating +/// storage gets the same "fix it once, every downstream comparison benefits" property +/// without either risk. +#[derive(Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. remote: SocketAddr, @@ -379,6 +390,51 @@ pub struct FourTuple { local_ip: Option, } +/// noq#738: canonicalizes an [`IpAddr`] for comparison purposes (see [`FourTuple`]'s +/// docs) without changing any stored/emitted address. +fn canonical_ip(ip: IpAddr) -> IpAddr { + ip.to_canonical() +} + +/// noq#738: canonical comparison key for a `remote: SocketAddr`. `to_canonical()` +/// folds `::ffff:a.b.c.d` down to plain IPv4; the scope_id is kept for addresses that +/// stay IPv6, because `FourTuple::new()` deliberately preserves it for link-local and +/// multicast remotes (see the comment there) — two different link-local interfaces +/// must not compare equal just because their canonicalized IP matches. A mapped +/// address canonicalizes to V4, which has no scope, so it gets 0. +pub(crate) fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { + let ip = addr.ip().to_canonical(); + let scope = match addr { + SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), + _ => 0, + }; + (ip, addr.port(), scope) +} + +/// noq#738: whether two `remote: SocketAddr`s refer to the same peer, ignoring the +/// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. Used +/// everywhere a raw `remote == remote` comparison would otherwise bypass +/// [`FourTuple`]'s canonicalizing `PartialEq`. +pub(crate) fn same_remote(a: SocketAddr, b: SocketAddr) -> bool { + remote_key(a) == remote_key(b) +} + +impl PartialEq for FourTuple { + fn eq(&self, other: &Self) -> bool { + remote_key(self.remote) == remote_key(other.remote) + && self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip) + } +} + +impl Eq for FourTuple {} + +impl std::hash::Hash for FourTuple { + fn hash(&self, state: &mut H) { + remote_key(self.remote).hash(state); + self.local_ip.map(canonical_ip).hash(state); + } +} + impl FourTuple { /// Creates a new [`FourTuple`]. pub fn new(mut remote: SocketAddr, local_ip: Option) -> Self { @@ -433,7 +489,10 @@ impl FourTuple { /// - `a.is_probably_same_path(b)` /// - `b.is_probably_same_path(a)` pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { - self.remote == other.remote && (self.local_ip.is_none() || self.local_ip == other.local_ip) + // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. + same_remote(self.remote, other.remote) + && (self.local_ip.is_none() + || self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip)) } } @@ -464,3 +523,41 @@ impl From for FourTuple { Self::from_remote(value) } } + +#[cfg(test)] +mod four_tuple_tests { + use std::collections::HashSet; + + use super::*; + + /// noq#738: two `FourTuple`s differing only by IPv4-mapped-IPv6 vs plain-IPv4 + /// representation must compare (and hash) equal. + #[test] + fn four_tuple_eq_ignores_mapped_v4_representation() { + let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); + let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); + assert_eq!(mapped, plain); + + let mut set = HashSet::new(); + set.insert(mapped); + set.insert(plain); + assert_eq!(set.len(), 1); + } + + /// noq#738 fix regression: two link-local `FourTuple`s that differ only in + /// `scope_id` (i.e. reachable via different network interfaces) must NOT + /// compare equal, even though their canonicalized IP is identical. An earlier + /// version of this fix dropped `scope_id` from the comparison entirely, which + /// silently collapsed distinct interfaces into a single path. + #[test] + fn four_tuple_eq_preserves_link_local_scope_id() { + let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); + assert_ne!(iface_a, iface_b); + + let mut set = HashSet::new(); + set.insert(iface_a); + set.insert(iface_b); + assert_eq!(set.len(), 2); + } +} diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 2b00fd542c..ef773fbf0a 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1,6 +1,6 @@ //! Tests for multipath -use std::net::SocketAddr; +use std::net::{Ipv6Addr, SocketAddr}; use std::num::NonZeroU32; use std::sync::Arc; use std::time::Duration; @@ -2293,3 +2293,74 @@ fn regression_discarded_path_stats_are_up_to_date() -> TestResult { Ok(()) } + +/// Regression test for issue #738. +/// +/// When a client opens a new path with an explicit `local_ip` set in the +/// [`FourTuple`], the path should validate successfully. On real devices the +/// `PATH_RESPONSE` was never matched to the outstanding `PATH_CHALLENGE` on +/// such paths, causing them to be abandoned with +/// [`PathAbandonReason::ValidationFailed`]. +/// +/// This test sets up a routing table where the client has a second interface +/// that can reach the *same* server address as the initial connection, and +/// opens a path on that second interface with an explicit `local_ip`. +/// +/// See +#[test] +fn open_path_with_explicit_local_ip() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + // Set up routing with a second client interface that can reach the same + // server as the first interface. Both server routes point to the same + // server address but link to different client interfaces, so the server + // can respond on either client interface. + let first_client_addr = pair.routes.as_basic().client_addr; + let server_addr = pair.routes.as_basic().server_addr; + let second_client_addr = { + let mut addr = first_client_addr; + if let SocketAddr::V6(v6) = &mut addr { + let s = v6.ip().segments(); + v6.set_ip(Ipv6Addr::new( + s[0], + s[1], + s[2], + s[3], + s[4], + s[5], + s[6], + s[7] + 1, + )); + } + addr + }; + + // `ManyToManyRouting::from_routes` rejects duplicate interface addresses + // (added by #721, after this test was originally written), which trips on + // the same `server_addr` appearing twice. `add_client_route`/`add_server_route` + // don't have that check, so build the same routing table incrementally instead. + let mut routing = ManyToManyRouting::simple_symmetric([first_client_addr], [server_addr]); + routing.add_client_route(second_client_addr, 0); + routing.add_server_route(server_addr, 1); + pair.routes = routing.into(); + + // Open a path with an explicit local_ip, targeting the same server as + // the initial connection (path 0). + let new_path = FourTuple::new(server_addr, Some(second_client_addr.ip())); + let path_id = pair.open_path(Client, new_path, PathStatus::Available)?; + pair.drive(); + + // The path should be established on both sides, not abandoned with + // ValidationFailed. + assert_matches!( + pair.poll(Client), + Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id + ); + + Ok(()) +} From 283640da7d720ba1bf28bf67596cdd93c36e29d6 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sat, 8 Aug 2026 02:57:17 -0500 Subject: [PATCH 03/20] refactor(proto): unify local_ip canonicalization behind same_local_ip() remote comparisons were already unified behind same_remote() in the previous commit, but local_ip comparisons were still open-coded three different ways (early_discard_packet, the passive-migration log check, and FourTuple's own PartialEq/is_probably_same_path). Same rationale as same_remote(): a single choke point means a future change can't fix some of these and miss the rest. No behavior change -- cargo test -p noq-proto: 390 passed, 0 failed. cargo test -p noq --lib: 32 passed, 3 ignored, 0 failed. --- noq-proto/src/connection/mod.rs | 14 +++----------- noq-proto/src/lib.rs | 13 +++++++++---- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 9ec5bca22f..487752c7a5 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -19,7 +19,7 @@ use tracing::{debug, error, trace, trace_span, warn}; use crate::{ Dir, Duration, EndpointConfig, FourTuple, Frame, INITIAL_MTU, Instant, MAX_CID_SIZE, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, Side, StreamId, TIMER_GRANULARITY, TokenStore, Transmit, - TransportError, TransportErrorCode, VarInt, same_remote, + TransportError, TransportErrorCode, VarInt, same_local_ip, same_remote, cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, config::{ServerConfig, TransportConfig}, @@ -2341,15 +2341,7 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && known_path - .network_path - .local_ip - .as_ref() - .map(std::net::IpAddr::to_canonical) - != network_path - .local_ip - .as_ref() - .map(std::net::IpAddr::to_canonical) + && !same_local_ip(known_path.network_path.local_ip, network_path.local_ip) && !local_ip_may_migrate { trace!( @@ -5579,7 +5571,7 @@ impl Connection { if path_data .network_path .local_ip - .is_some_and(|ip| ip.to_canonical() != new_local_ip.to_canonical()) + .is_some_and(|ip| !same_local_ip(Some(ip), Some(new_local_ip))) { debug!( %path_id, diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 2012a8a6b2..ac6c71d3c3 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -419,10 +419,16 @@ pub(crate) fn same_remote(a: SocketAddr, b: SocketAddr) -> bool { remote_key(a) == remote_key(b) } +/// noq#738: same rationale as [`same_remote`], for `local_ip: Option` +/// comparisons. Used everywhere a raw `local_ip == local_ip` comparison would +/// otherwise bypass [`FourTuple`]'s canonicalizing `PartialEq`. +pub(crate) fn same_local_ip(a: Option, b: Option) -> bool { + a.map(canonical_ip) == b.map(canonical_ip) +} + impl PartialEq for FourTuple { fn eq(&self, other: &Self) -> bool { - remote_key(self.remote) == remote_key(other.remote) - && self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip) + same_remote(self.remote, other.remote) && same_local_ip(self.local_ip, other.local_ip) } } @@ -491,8 +497,7 @@ impl FourTuple { pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. same_remote(self.remote, other.remote) - && (self.local_ip.is_none() - || self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip)) + && (self.local_ip.is_none() || same_local_ip(self.local_ip, other.local_ip)) } } From 93f0ecb95e31b5802bd20acd0785a0edd2a76d2d Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sat, 8 Aug 2026 03:03:41 -0500 Subject: [PATCH 04/20] test(proto): cover is_probably_same_path and the scope_id edge cases Adds direct tests for is_probably_same_path (previously only exercised transitively via FourTuple's PartialEq, but it's a separate implementation with its own asymmetric local_ip rule, so it can't just delegate to ==) plus two gaps in the scope_id matrix that the earlier scope_id fix didn't lock in: - four_tuple_eq_zeroes_scope_id_for_global_v6: the inverse of the link-local test -- a *global* IPv6 address with a bogus/differing scope_id must still compare equal, since FourTuple::new() zeroes scope_id for anything that isn't link-local/multicast. Guards against a future "fix" that makes remote_key() preserve scope_id unconditionally. - four_tuple_eq_preserves_multicast_scope_id: mirrors the link-local test for the other half of FourTuple::new()'s requires_scope_id condition. cargo test -p noq-proto: 394 passed, 0 failed. cargo test -p noq --lib: 32 passed, 3 ignored, 0 failed. --- noq-proto/src/lib.rs | 60 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index ac6c71d3c3..445f0cf36e 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -565,4 +565,64 @@ mod four_tuple_tests { set.insert(iface_b); assert_eq!(set.len(), 2); } + + /// The inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a + /// *global* (non-link-local, non-multicast) IPv6 address, `FourTuple::new()` + /// deliberately zeroes `scope_id` before storing (it's only meaningful for + /// link-local/multicast scopes). Two `FourTuple`s built from the same global + /// address but with different (bogus/leftover) scope_ids on the input must + /// still compare equal — if a future change made `remote_key()` preserve + /// scope_id unconditionally instead of relying on `new()` having already + /// zeroed it, this would start failing silently. + #[test] + fn four_tuple_eq_zeroes_scope_id_for_global_v6() { + let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); + let b = FourTuple::from_remote("[2001:db8::1%5]:443".parse().unwrap()); + assert_eq!(a, b); + + let mut set = HashSet::new(); + set.insert(a); + set.insert(b); + assert_eq!(set.len(), 1); + } + + /// Mirrors [`four_tuple_eq_preserves_link_local_scope_id`] for the other half + /// of `FourTuple::new()`'s `requires_scope_id` condition (multicast, not just + /// unicast link-local). + #[test] + fn four_tuple_eq_preserves_multicast_scope_id() { + let iface_a = FourTuple::from_remote("[ff02::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[ff02::1%5]:443".parse().unwrap()); + assert_ne!(iface_a, iface_b); + + let mut set = HashSet::new(); + set.insert(iface_a); + set.insert(iface_b); + assert_eq!(set.len(), 2); + } + + /// [`FourTuple::is_probably_same_path`] duplicates the canonicalizing + /// comparison independently of the derived-then-hand-written `PartialEq` + /// (it has its own asymmetric "only compare local_ip if self has one" rule, + /// so it can't just delegate to `==`) -- test it directly so a future change + /// can't fix one and silently leave the other on raw comparison. + #[test] + fn is_probably_same_path_ignores_mapped_v4_representation() { + let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); + let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); + assert!(mapped.is_probably_same_path(&plain)); + assert!(plain.is_probably_same_path(&mapped)); + } + + /// Same as above, for the `scope_id` half of the fix. `scope_id` only exists + /// on `remote: SocketAddr` (`Ipv6Addr`/`IpAddr` -- and so `local_ip` -- has no + /// such field; `"...%3".parse::()` doesn't even parse), so this + /// varies `remote`, not `local_ip`. + #[test] + fn is_probably_same_path_distinguishes_link_local_scope_id() { + let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); + let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); + assert!(!iface_a.is_probably_same_path(&iface_b)); + assert!(!iface_b.is_probably_same_path(&iface_a)); + } } From ae9ebc5e95c04ed977422a72d899862107284f90 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sat, 8 Aug 2026 03:41:56 -0500 Subject: [PATCH 05/20] fix(proto): correct four_tuple_eq_zeroes_scope_id_for_global_v6's doc comment It claimed to guard against a future remote_key() change preserving scope_id unconditionally, but remote_key() never sees a nonzero scope_id for a global address in the first place -- FourTuple::new() already zeroes it before storage. Verified by mutation (opus review): removing remote_key()'s is_ipv6() guard still passes all 6 four_tuple_tests. The test itself is still worth keeping (it locks in new()'s zeroing), just the stated rationale was wrong. Also tightened the two is_probably_same_path tests' doc comments: they test the remote-canonicalization half only (both sides have local_ip: None), not the asymmetric local_ip rule the previous wording implied. --- noq-proto/src/lib.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 445f0cf36e..5d60b52217 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -569,11 +569,12 @@ mod four_tuple_tests { /// The inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a /// *global* (non-link-local, non-multicast) IPv6 address, `FourTuple::new()` /// deliberately zeroes `scope_id` before storing (it's only meaningful for - /// link-local/multicast scopes). Two `FourTuple`s built from the same global - /// address but with different (bogus/leftover) scope_ids on the input must - /// still compare equal — if a future change made `remote_key()` preserve - /// scope_id unconditionally instead of relying on `new()` having already - /// zeroed it, this would start failing silently. + /// link-local/multicast scopes) -- so by the time `remote_key()` runs, a + /// global address's `scope_id` is already 0 regardless of what was on the + /// input. This locks in that zeroing: two `FourTuple`s built from the same + /// global address but with different (bogus/leftover) scope_ids on the input + /// must still compare equal. If `FourTuple::new()` ever stopped zeroing + /// scope_id for global addresses, this would start failing. #[test] fn four_tuple_eq_zeroes_scope_id_for_global_v6() { let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); @@ -602,10 +603,14 @@ mod four_tuple_tests { } /// [`FourTuple::is_probably_same_path`] duplicates the canonicalizing - /// comparison independently of the derived-then-hand-written `PartialEq` - /// (it has its own asymmetric "only compare local_ip if self has one" rule, - /// so it can't just delegate to `==`) -- test it directly so a future change - /// can't fix one and silently leave the other on raw comparison. + /// `remote` comparison independently of the derived-then-hand-written + /// `PartialEq` (it can't just delegate to `==`, since it has its own + /// asymmetric "only compare local_ip if self has one" rule on top) -- test + /// the remote-canonicalization half directly so a future change can't fix + /// `PartialEq` and silently leave this one on a raw comparison. Both sides + /// have `local_ip: None` here, so this doesn't exercise the asymmetric rule + /// itself -- that's covered transitively elsewhere (e.g. connection-level + /// tests), not by this pair. #[test] fn is_probably_same_path_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); From c703cc42d5c6578e313f7abe3f950680588891f4 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 05:37:32 -0500 Subject: [PATCH 06/20] refactor(proto): move same_remote/same_local_ip onto FourTuple as methods Per review feedback on #784: replace the free-standing same_remote(a, b)/ same_local_ip(a, b) helpers (which took raw SocketAddr/Option and required callers to unpack FourTuple fields by hand) with FourTuple::same_remote(&self, other: &Self) and FourTuple::same_local_ip(&self, other: &Self). All call sites already had two FourTuples in scope, so this is a straightforward substitution; PartialEq/is_probably_same_path now delegate to the same methods instead of duplicating the comparison logic. --- noq-proto/src/connection/mod.rs | 36 +++++++++++++++++-------------- noq-proto/src/connection/paths.rs | 7 +++--- noq-proto/src/lib.rs | 36 ++++++++++++++----------------- 3 files changed, 39 insertions(+), 40 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 487752c7a5..c80902bf27 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -19,7 +19,7 @@ use tracing::{debug, error, trace, trace_span, warn}; use crate::{ Dir, Duration, EndpointConfig, FourTuple, Frame, INITIAL_MTU, Instant, MAX_CID_SIZE, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, Side, StreamId, TIMER_GRANULARITY, TokenStore, Transmit, - TransportError, TransportErrorCode, VarInt, same_local_ip, same_remote, + TransportError, TransportErrorCode, VarInt, cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, config::{ServerConfig, TransportConfig}, @@ -2327,9 +2327,10 @@ impl Connection { // forbids migration, drop the datagram. This could be relaxed to heuristically // permit NAT-rebinding-like migration. if let Some(known_path) = self.path_mut(path_id) { - // noq#738: canonicalize both sides, same rationale as the local_ip comparison - // below and `FourTuple`'s `PartialEq`. - if !same_remote(network_path.remote, known_path.network_path.remote) && !peer_may_probe { + // noq#738: `FourTuple::same_remote` canonicalizes mapped-IPv4-vs-plain-IPv4 + // representations, same rationale as the local_ip comparison below and + // `FourTuple`'s `PartialEq`. + if !network_path.same_remote(&known_path.network_path) && !peer_may_probe { trace!( %path_id, %network_path, @@ -2341,7 +2342,7 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && !same_local_ip(known_path.network_path.local_ip, network_path.local_ip) + && !network_path.same_local_ip(&known_path.network_path) && !local_ip_may_migrate { trace!( @@ -4987,8 +4988,8 @@ impl Connection { let path = &mut self .path_mut(path_id) .expect("payload is processed only after the path becomes known"); - // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. - if same_remote(network_path.remote, path.network_path.remote) { + // noq#738: same rationale as `FourTuple`'s `PartialEq`. + if network_path.same_remote(&path.network_path) { // PATH_CHALLENGE on active path, possible off-path packet // forwarding attack. Send a non-probing packet to recover the // active path. See @@ -5285,8 +5286,8 @@ impl Connection { let space_open_status = self.spaces[SpaceKind::Data].for_path(path_id).open_status; let path = self.path_data_mut(path_id); - // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. - if same_remote(path.network_path.remote, network_path.remote) { + // noq#738: same rationale as `FourTuple`'s `PartialEq`. + if path.network_path.same_remote(&network_path) { if let Some(updated) = path.update_observed_addr_report(observed) && space_open_status == OpenStatus::Informed { @@ -5567,11 +5568,11 @@ impl Connection { && let Some(new_local_ip) = network_path.local_ip { let path_data = self.path_data_mut(path_id); - // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. - if path_data - .network_path - .local_ip - .is_some_and(|ip| !same_local_ip(Some(ip), Some(new_local_ip))) + // noq#738: same rationale as `FourTuple`'s `PartialEq`. `network_path.local_ip` + // is `Some(new_local_ip)` per the `let Some` guard above, so comparing against + // `network_path` itself covers the `new_local_ip` side. + if path_data.network_path.local_ip.is_some() + && !path_data.network_path.same_local_ip(&network_path) { debug!( %path_id, @@ -5584,11 +5585,14 @@ impl Connection { } // If the peer migrated to a new address, trigger migration. - // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. + // noq#738: same rationale as `FourTuple`'s `PartialEq`. if self.peer_may_migrate() && (migrate_on_any_packet || !is_probing_packet) && is_largest_received_pn - && !same_remote(network_path.remote, self.path_data(path_id).network_path.remote) + && !self + .path_data(path_id) + .network_path + .same_remote(&network_path) { self.migrate(path_id, now, network_path, migration_observed_addr); // Break linkability, if possible diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index c6eab62a36..17c877ecf3 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -12,7 +12,7 @@ use super::{ }; use crate::{ ConnectionId, Duration, FourTuple, Instant, TIMER_GRANULARITY, TransportConfig, - TransportErrorCode, VarInt, same_remote, + TransportErrorCode, VarInt, coding::{self, Decodable, Encodable}, congestion, connection::{MAX_BACKOFF_EXPONENT, MAX_PTO_INTERVAL}, @@ -872,12 +872,11 @@ impl PathResponses { token, network_path, }; - // noq#738: canonicalize both sides before comparing, same rationale as - // `FourTuple`'s `PartialEq`. + // noq#738: same rationale as `FourTuple`'s `PartialEq`. let existing = self .pending .iter_mut() - .find(|x| same_remote(x.network_path.remote, network_path.remote)); + .find(|x| x.network_path.same_remote(&network_path)); if let Some(existing) = existing { // Update a queued response if existing.packet <= packet { diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 5d60b52217..a07bfdb1df 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -402,7 +402,7 @@ fn canonical_ip(ip: IpAddr) -> IpAddr { /// multicast remotes (see the comment there) — two different link-local interfaces /// must not compare equal just because their canonicalized IP matches. A mapped /// address canonicalizes to V4, which has no scope, so it gets 0. -pub(crate) fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { +fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { let ip = addr.ip().to_canonical(); let scope = match addr { SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), @@ -411,24 +411,9 @@ pub(crate) fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { (ip, addr.port(), scope) } -/// noq#738: whether two `remote: SocketAddr`s refer to the same peer, ignoring the -/// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. Used -/// everywhere a raw `remote == remote` comparison would otherwise bypass -/// [`FourTuple`]'s canonicalizing `PartialEq`. -pub(crate) fn same_remote(a: SocketAddr, b: SocketAddr) -> bool { - remote_key(a) == remote_key(b) -} - -/// noq#738: same rationale as [`same_remote`], for `local_ip: Option` -/// comparisons. Used everywhere a raw `local_ip == local_ip` comparison would -/// otherwise bypass [`FourTuple`]'s canonicalizing `PartialEq`. -pub(crate) fn same_local_ip(a: Option, b: Option) -> bool { - a.map(canonical_ip) == b.map(canonical_ip) -} - impl PartialEq for FourTuple { fn eq(&self, other: &Self) -> bool { - same_remote(self.remote, other.remote) && same_local_ip(self.local_ip, other.local_ip) + self.same_remote(other) && self.same_local_ip(other) } } @@ -484,6 +469,19 @@ impl FourTuple { self.local_ip } + /// noq#738: whether `self` and `other` share the same remote peer, ignoring the + /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. + /// Used everywhere a raw `remote == remote` comparison would otherwise bypass + /// this canonicalization (see the type-level docs above). + pub(crate) fn same_remote(&self, other: &Self) -> bool { + remote_key(self.remote) == remote_key(other.remote) + } + + /// noq#738: same rationale as [`Self::same_remote`], for `local_ip`. + pub(crate) fn same_local_ip(&self, other: &Self) -> bool { + self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip) + } + /// Returns whether we think the other address probably represents the same path /// as ours. /// @@ -495,9 +493,7 @@ impl FourTuple { /// - `a.is_probably_same_path(b)` /// - `b.is_probably_same_path(a)` pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { - // noq#738: canonicalize both sides, same rationale as `FourTuple`'s `PartialEq`. - same_remote(self.remote, other.remote) - && (self.local_ip.is_none() || same_local_ip(self.local_ip, other.local_ip)) + self.same_remote(other) && (self.local_ip.is_none() || self.same_local_ip(other)) } } From 3f982b82e1771bf5d64b6b4cc78b045ebfde6bd9 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 05:40:09 -0500 Subject: [PATCH 07/20] rename same_remote/same_local_ip to is_same_remote/is_same_local_ip Matches this crate's existing naming convention for two-instance boolean predicates (FourTuple::is_probably_same_path, Connection::is_same_connection in noq/src/connection.rs), rather than the bare same_x form. --- noq-proto/src/connection/mod.rs | 14 +++++++------- noq-proto/src/connection/paths.rs | 2 +- noq-proto/src/lib.rs | 10 +++++----- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index c80902bf27..2c7ae4d694 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2327,10 +2327,10 @@ impl Connection { // forbids migration, drop the datagram. This could be relaxed to heuristically // permit NAT-rebinding-like migration. if let Some(known_path) = self.path_mut(path_id) { - // noq#738: `FourTuple::same_remote` canonicalizes mapped-IPv4-vs-plain-IPv4 + // noq#738: `FourTuple::is_same_remote` canonicalizes mapped-IPv4-vs-plain-IPv4 // representations, same rationale as the local_ip comparison below and // `FourTuple`'s `PartialEq`. - if !network_path.same_remote(&known_path.network_path) && !peer_may_probe { + if !network_path.is_same_remote(&known_path.network_path) && !peer_may_probe { trace!( %path_id, %network_path, @@ -2342,7 +2342,7 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && !network_path.same_local_ip(&known_path.network_path) + && !network_path.is_same_local_ip(&known_path.network_path) && !local_ip_may_migrate { trace!( @@ -4989,7 +4989,7 @@ impl Connection { .path_mut(path_id) .expect("payload is processed only after the path becomes known"); // noq#738: same rationale as `FourTuple`'s `PartialEq`. - if network_path.same_remote(&path.network_path) { + if network_path.is_same_remote(&path.network_path) { // PATH_CHALLENGE on active path, possible off-path packet // forwarding attack. Send a non-probing packet to recover the // active path. See @@ -5287,7 +5287,7 @@ impl Connection { self.spaces[SpaceKind::Data].for_path(path_id).open_status; let path = self.path_data_mut(path_id); // noq#738: same rationale as `FourTuple`'s `PartialEq`. - if path.network_path.same_remote(&network_path) { + if path.network_path.is_same_remote(&network_path) { if let Some(updated) = path.update_observed_addr_report(observed) && space_open_status == OpenStatus::Informed { @@ -5572,7 +5572,7 @@ impl Connection { // is `Some(new_local_ip)` per the `let Some` guard above, so comparing against // `network_path` itself covers the `new_local_ip` side. if path_data.network_path.local_ip.is_some() - && !path_data.network_path.same_local_ip(&network_path) + && !path_data.network_path.is_same_local_ip(&network_path) { debug!( %path_id, @@ -5592,7 +5592,7 @@ impl Connection { && !self .path_data(path_id) .network_path - .same_remote(&network_path) + .is_same_remote(&network_path) { self.migrate(path_id, now, network_path, migration_observed_addr); // Break linkability, if possible diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index 17c877ecf3..2a3998c3e1 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -876,7 +876,7 @@ impl PathResponses { let existing = self .pending .iter_mut() - .find(|x| x.network_path.same_remote(&network_path)); + .find(|x| x.network_path.is_same_remote(&network_path)); if let Some(existing) = existing { // Update a queued response if existing.packet <= packet { diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index a07bfdb1df..eec3bc2cad 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -413,7 +413,7 @@ fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { impl PartialEq for FourTuple { fn eq(&self, other: &Self) -> bool { - self.same_remote(other) && self.same_local_ip(other) + self.is_same_remote(other) && self.is_same_local_ip(other) } } @@ -473,12 +473,12 @@ impl FourTuple { /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. /// Used everywhere a raw `remote == remote` comparison would otherwise bypass /// this canonicalization (see the type-level docs above). - pub(crate) fn same_remote(&self, other: &Self) -> bool { + pub(crate) fn is_same_remote(&self, other: &Self) -> bool { remote_key(self.remote) == remote_key(other.remote) } - /// noq#738: same rationale as [`Self::same_remote`], for `local_ip`. - pub(crate) fn same_local_ip(&self, other: &Self) -> bool { + /// noq#738: same rationale as [`Self::is_same_remote`], for `local_ip`. + pub(crate) fn is_same_local_ip(&self, other: &Self) -> bool { self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip) } @@ -493,7 +493,7 @@ impl FourTuple { /// - `a.is_probably_same_path(b)` /// - `b.is_probably_same_path(a)` pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { - self.same_remote(other) && (self.local_ip.is_none() || self.same_local_ip(other)) + self.is_same_remote(other) && (self.local_ip.is_none() || self.is_same_local_ip(other)) } } From 3986b6b2532eb64ba0b495a1c4b848f6cfc389fd Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 05:48:22 -0500 Subject: [PATCH 08/20] refactor(proto): tidy up FourTuple canonicalization helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fold canonical_ip/remote_key free functions into private FourTuple methods (remote_key/local_ip_key), consistent with turning same_remote/same_local_ip into methods in the previous commit — they're as much an implementation detail of FourTuple's fields as the comparison predicates that use them. - Move the hand-written PartialEq/Eq/Hash impls to after the inherent impl FourTuple block (matching this file's existing struct -> inherent impl -> trait impl ordering for Side/Dir), so is_same_remote/ is_same_local_ip are defined before they're referenced. - Drop the repeated "noq#738: same rationale as FourTuple's PartialEq" comments at each call site now that the method names themselves carry that context; kept the one comment that explains a non-obvious invariant (network_path.local_ip being guaranteed Some(new_local_ip)). --- noq-proto/src/connection/mod.rs | 12 ++--- noq-proto/src/connection/paths.rs | 1 - noq-proto/src/lib.rs | 76 +++++++++++++++---------------- 3 files changed, 41 insertions(+), 48 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 2c7ae4d694..4419cc2f29 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2327,9 +2327,6 @@ impl Connection { // forbids migration, drop the datagram. This could be relaxed to heuristically // permit NAT-rebinding-like migration. if let Some(known_path) = self.path_mut(path_id) { - // noq#738: `FourTuple::is_same_remote` canonicalizes mapped-IPv4-vs-plain-IPv4 - // representations, same rationale as the local_ip comparison below and - // `FourTuple`'s `PartialEq`. if !network_path.is_same_remote(&known_path.network_path) && !peer_may_probe { trace!( %path_id, @@ -4988,7 +4985,6 @@ impl Connection { let path = &mut self .path_mut(path_id) .expect("payload is processed only after the path becomes known"); - // noq#738: same rationale as `FourTuple`'s `PartialEq`. if network_path.is_same_remote(&path.network_path) { // PATH_CHALLENGE on active path, possible off-path packet // forwarding attack. Send a non-probing packet to recover the @@ -5286,7 +5282,6 @@ impl Connection { let space_open_status = self.spaces[SpaceKind::Data].for_path(path_id).open_status; let path = self.path_data_mut(path_id); - // noq#738: same rationale as `FourTuple`'s `PartialEq`. if path.network_path.is_same_remote(&network_path) { if let Some(updated) = path.update_observed_addr_report(observed) && space_open_status == OpenStatus::Informed @@ -5568,9 +5563,9 @@ impl Connection { && let Some(new_local_ip) = network_path.local_ip { let path_data = self.path_data_mut(path_id); - // noq#738: same rationale as `FourTuple`'s `PartialEq`. `network_path.local_ip` - // is `Some(new_local_ip)` per the `let Some` guard above, so comparing against - // `network_path` itself covers the `new_local_ip` side. + // `network_path.local_ip` is `Some(new_local_ip)` per the `let Some` guard + // above, so comparing against `network_path` itself covers the + // `new_local_ip` side. if path_data.network_path.local_ip.is_some() && !path_data.network_path.is_same_local_ip(&network_path) { @@ -5585,7 +5580,6 @@ impl Connection { } // If the peer migrated to a new address, trigger migration. - // noq#738: same rationale as `FourTuple`'s `PartialEq`. if self.peer_may_migrate() && (migrate_on_any_packet || !is_probing_packet) && is_largest_received_pn diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index 2a3998c3e1..e8b6fd4997 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -872,7 +872,6 @@ impl PathResponses { token, network_path, }; - // noq#738: same rationale as `FourTuple`'s `PartialEq`. let existing = self .pending .iter_mut() diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index eec3bc2cad..a216a01643 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -390,42 +390,6 @@ pub struct FourTuple { local_ip: Option, } -/// noq#738: canonicalizes an [`IpAddr`] for comparison purposes (see [`FourTuple`]'s -/// docs) without changing any stored/emitted address. -fn canonical_ip(ip: IpAddr) -> IpAddr { - ip.to_canonical() -} - -/// noq#738: canonical comparison key for a `remote: SocketAddr`. `to_canonical()` -/// folds `::ffff:a.b.c.d` down to plain IPv4; the scope_id is kept for addresses that -/// stay IPv6, because `FourTuple::new()` deliberately preserves it for link-local and -/// multicast remotes (see the comment there) — two different link-local interfaces -/// must not compare equal just because their canonicalized IP matches. A mapped -/// address canonicalizes to V4, which has no scope, so it gets 0. -fn remote_key(addr: SocketAddr) -> (IpAddr, u16, u32) { - let ip = addr.ip().to_canonical(); - let scope = match addr { - SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), - _ => 0, - }; - (ip, addr.port(), scope) -} - -impl PartialEq for FourTuple { - fn eq(&self, other: &Self) -> bool { - self.is_same_remote(other) && self.is_same_local_ip(other) - } -} - -impl Eq for FourTuple {} - -impl std::hash::Hash for FourTuple { - fn hash(&self, state: &mut H) { - remote_key(self.remote).hash(state); - self.local_ip.map(canonical_ip).hash(state); - } -} - impl FourTuple { /// Creates a new [`FourTuple`]. pub fn new(mut remote: SocketAddr, local_ip: Option) -> Self { @@ -469,17 +433,38 @@ impl FourTuple { self.local_ip } + /// noq#738: canonical comparison key for `remote`. `to_canonical()` folds + /// `::ffff:a.b.c.d` down to plain IPv4; the scope_id is kept for addresses that + /// stay IPv6, because [`Self::new`] deliberately preserves it for link-local and + /// multicast remotes — two different link-local interfaces must not compare + /// equal just because their canonicalized IP matches. A mapped address + /// canonicalizes to V4, which has no scope, so it gets 0. + fn remote_key(&self) -> (IpAddr, u16, u32) { + let ip = self.remote.ip().to_canonical(); + let scope = match self.remote { + SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), + _ => 0, + }; + (ip, self.remote.port(), scope) + } + + /// noq#738: canonical comparison key for `local_ip`, same rationale as + /// [`Self::remote_key`]. + fn local_ip_key(&self) -> Option { + self.local_ip.map(|ip| ip.to_canonical()) + } + /// noq#738: whether `self` and `other` share the same remote peer, ignoring the /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. /// Used everywhere a raw `remote == remote` comparison would otherwise bypass /// this canonicalization (see the type-level docs above). pub(crate) fn is_same_remote(&self, other: &Self) -> bool { - remote_key(self.remote) == remote_key(other.remote) + self.remote_key() == other.remote_key() } /// noq#738: same rationale as [`Self::is_same_remote`], for `local_ip`. pub(crate) fn is_same_local_ip(&self, other: &Self) -> bool { - self.local_ip.map(canonical_ip) == other.local_ip.map(canonical_ip) + self.local_ip_key() == other.local_ip_key() } /// Returns whether we think the other address probably represents the same path @@ -497,6 +482,21 @@ impl FourTuple { } } +impl PartialEq for FourTuple { + fn eq(&self, other: &Self) -> bool { + self.is_same_remote(other) && self.is_same_local_ip(other) + } +} + +impl Eq for FourTuple {} + +impl std::hash::Hash for FourTuple { + fn hash(&self, state: &mut H) { + self.remote_key().hash(state); + self.local_ip_key().hash(state); + } +} + impl fmt::Display for FourTuple { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("(local: ")?; From c9ff6a327058471fb85f56085a3b6b0da30755e5 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 06:28:33 -0500 Subject: [PATCH 09/20] rename remote_key/local_ip_key to canonical_remote/canonical_local_ip _key already means something else in this crate (HmacKey/HandshakeTokenKey in config/mod.rs, cid_generator::from_key) -- reusing it here for "value used for comparison purposes" invited confusion with those literal cryptographic keys. canonical_remote/canonical_local_ip instead matches this crate's own existing vocabulary for this exact kind of operation (n0_nat_traversal::CanonicalIpPort::as_canonical_addr, IpAddr::to_canonical which these methods wrap). --- noq-proto/src/lib.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index a216a01643..48e1f19725 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -433,13 +433,14 @@ impl FourTuple { self.local_ip } - /// noq#738: canonical comparison key for `remote`. `to_canonical()` folds - /// `::ffff:a.b.c.d` down to plain IPv4; the scope_id is kept for addresses that - /// stay IPv6, because [`Self::new`] deliberately preserves it for link-local and - /// multicast remotes — two different link-local interfaces must not compare - /// equal just because their canonicalized IP matches. A mapped address - /// canonicalizes to V4, which has no scope, so it gets 0. - fn remote_key(&self) -> (IpAddr, u16, u32) { + /// noq#738: canonicalizes `remote` for comparison/hashing purposes (see the + /// type-level docs above). `to_canonical()` folds `::ffff:a.b.c.d` down to plain + /// IPv4; the scope_id is kept for addresses that stay IPv6, because [`Self::new`] + /// deliberately preserves it for link-local and multicast remotes — two different + /// link-local interfaces must not compare equal just because their canonicalized + /// IP matches. A mapped address canonicalizes to V4, which has no scope, so it + /// gets 0. + fn canonical_remote(&self) -> (IpAddr, u16, u32) { let ip = self.remote.ip().to_canonical(); let scope = match self.remote { SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), @@ -448,9 +449,8 @@ impl FourTuple { (ip, self.remote.port(), scope) } - /// noq#738: canonical comparison key for `local_ip`, same rationale as - /// [`Self::remote_key`]. - fn local_ip_key(&self) -> Option { + /// noq#738: same rationale as [`Self::canonical_remote`], for `local_ip`. + fn canonical_local_ip(&self) -> Option { self.local_ip.map(|ip| ip.to_canonical()) } @@ -459,12 +459,12 @@ impl FourTuple { /// Used everywhere a raw `remote == remote` comparison would otherwise bypass /// this canonicalization (see the type-level docs above). pub(crate) fn is_same_remote(&self, other: &Self) -> bool { - self.remote_key() == other.remote_key() + self.canonical_remote() == other.canonical_remote() } /// noq#738: same rationale as [`Self::is_same_remote`], for `local_ip`. pub(crate) fn is_same_local_ip(&self, other: &Self) -> bool { - self.local_ip_key() == other.local_ip_key() + self.canonical_local_ip() == other.canonical_local_ip() } /// Returns whether we think the other address probably represents the same path @@ -492,8 +492,8 @@ impl Eq for FourTuple {} impl std::hash::Hash for FourTuple { fn hash(&self, state: &mut H) { - self.remote_key().hash(state); - self.local_ip_key().hash(state); + self.canonical_remote().hash(state); + self.canonical_local_ip().hash(state); } } @@ -565,7 +565,7 @@ mod four_tuple_tests { /// The inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a /// *global* (non-link-local, non-multicast) IPv6 address, `FourTuple::new()` /// deliberately zeroes `scope_id` before storing (it's only meaningful for - /// link-local/multicast scopes) -- so by the time `remote_key()` runs, a + /// link-local/multicast scopes) -- so by the time `canonical_remote()` runs, a /// global address's `scope_id` is already 0 regardless of what was on the /// input. This locks in that zeroing: two `FourTuple`s built from the same /// global address but with different (bogus/leftover) scope_ids on the input From a7866e0c28d848a7988f62439bf4334e1d5c1439 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 07:42:20 -0500 Subject: [PATCH 10/20] test(proto): cover local_ip canonicalization and PathResponses::push Independent review flagged two gaps in the existing regression coverage: - All existing four_tuple_tests used FourTuple::from_remote (local_ip: None), so canonical_local_ip/is_same_local_ip's Some(..) path -- the half of the original bug report that open_path()'s local_ip argument actually depends on -- was never exercised. Added four_tuple_eq_ignores_mapped_v4_representation_for_local_ip and is_probably_same_path_ignores_mapped_v4_representation_for_local_ip to close that. - None of the 5 call-site fixes in connection/mod.rs and connection/paths.rs were pinned by any test (verified: reverting all 5 to their pre-fix raw comparisons still passed the full suite). PathResponses::push is the one call site cheaply testable in isolation (pub(crate), no live Connection/handshake needed) -- added push_coalesces_mapped_v4_representation. The other 4 call sites are on private Connection methods only reachable through full E2E simulation; covering those would need new routing-test infrastructure to simulate a representation mismatch, which is out of scope here (already verified manually on real hardware per the PR description). All three new tests verified to fail without the corresponding fix. --- noq-proto/src/connection/paths.rs | 25 +++++++++++++++++++ noq-proto/src/lib.rs | 40 ++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index e8b6fd4997..e0676a350e 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -1202,4 +1202,29 @@ mod tests { // outside range saturates assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX) } + + /// noq#738 regression: two `PATH_CHALLENGE`s arriving on `FourTuple`s that + /// differ only by mapped-IPv4-vs-plain-IPv4 representation of the same peer + /// must be coalesced into a single pending response, not tracked as two + /// distinct (and therefore never-matching) paths. + #[test] + fn push_coalesces_mapped_v4_representation() { + let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); + let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); + + let mut responses = PathResponses::default(); + responses.push(1, 0xaaaa, mapped); + responses.push(2, 0xbbbb, plain); + + assert_eq!( + responses.pending.len(), + 1, + "the same peer under different address representations must coalesce \ + into a single pending response" + ); + assert_eq!( + responses.pending[0].token, 0xbbbb, + "the later (higher packet number) response should have been queued" + ); + } } diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 48e1f19725..8d3eb41f6b 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -545,6 +545,24 @@ mod four_tuple_tests { assert_eq!(set.len(), 1); } + /// Same as [`four_tuple_eq_ignores_mapped_v4_representation`], but for + /// `local_ip` instead of `remote` -- this is the half of the fix that the + /// original bug report actually depends on (`open_path()`'s `local_ip` + /// argument), and unlike `remote` it's never exercised by + /// `FourTuple::from_remote` (which always sets `local_ip: None`). + #[test] + fn four_tuple_eq_ignores_mapped_v4_representation_for_local_ip() { + let remote = "9.9.9.9:443".parse().unwrap(); + let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); + let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); + assert_eq!(mapped, plain); + + let mut set = HashSet::new(); + set.insert(mapped); + set.insert(plain); + assert_eq!(set.len(), 1); + } + /// noq#738 fix regression: two link-local `FourTuple`s that differ only in /// `scope_id` (i.e. reachable via different network interfaces) must NOT /// compare equal, even though their canonicalized IP is identical. An earlier @@ -604,9 +622,10 @@ mod four_tuple_tests { /// asymmetric "only compare local_ip if self has one" rule on top) -- test /// the remote-canonicalization half directly so a future change can't fix /// `PartialEq` and silently leave this one on a raw comparison. Both sides - /// have `local_ip: None` here, so this doesn't exercise the asymmetric rule - /// itself -- that's covered transitively elsewhere (e.g. connection-level - /// tests), not by this pair. + /// have `local_ip: None` here, so this doesn't exercise the asymmetric + /// `local_ip` rule itself -- see + /// [`is_probably_same_path_ignores_mapped_v4_representation_for_local_ip`] + /// for that. #[test] fn is_probably_same_path_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); @@ -615,6 +634,21 @@ mod four_tuple_tests { assert!(plain.is_probably_same_path(&mapped)); } + /// Exercises the asymmetric `local_ip` branch that + /// [`is_probably_same_path_ignores_mapped_v4_representation`] deliberately + /// leaves untested: when `self.local_ip` is `Some(..)`, + /// `is_probably_same_path` requires the full (canonicalizing) `FourTuple` + /// equality, not just a `remote` match. A representation-only difference in + /// `local_ip` must not break that. + #[test] + fn is_probably_same_path_ignores_mapped_v4_representation_for_local_ip() { + let remote = "9.9.9.9:443".parse().unwrap(); + let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); + let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); + assert!(mapped.is_probably_same_path(&plain)); + assert!(plain.is_probably_same_path(&mapped)); + } + /// Same as above, for the `scope_id` half of the fix. `scope_id` only exists /// on `remote: SocketAddr` (`Ipv6Addr`/`IpAddr` -- and so `local_ip` -- has no /// such field; `"...%3".parse::()` doesn't even parse), so this From 4efcd676bc9b66b1b1ed5e3fc42a28f5a5c020b2 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Sun, 9 Aug 2026 07:47:51 -0500 Subject: [PATCH 11/20] docs(proto): trim test doc comments to match this file's usual brevity The four_tuple_tests/PathResponses test comments had grown to 4-9 lines each, several just restating what the test's own name already says. Existing regression tests elsewhere in this crate (e.g. regression_path_validation_stale_local_after_passive_migration in tests/mod.rs) keep this to 2-3 lines -- summary + the one non-obvious reason (history, cross-reference, or asymmetry) it exists. Trimmed to match; no content with actual information value was removed. --- noq-proto/src/connection/paths.rs | 18 ++------ noq-proto/src/lib.rs | 68 +++++++++++-------------------- 2 files changed, 28 insertions(+), 58 deletions(-) diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index e0676a350e..c82f0912c7 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -1203,10 +1203,8 @@ mod tests { assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX) } - /// noq#738 regression: two `PATH_CHALLENGE`s arriving on `FourTuple`s that - /// differ only by mapped-IPv4-vs-plain-IPv4 representation of the same peer - /// must be coalesced into a single pending response, not tracked as two - /// distinct (and therefore never-matching) paths. + /// noq#738 regression: `PATH_CHALLENGE`s on mapped-vs-plain-IPv4 FourTuples of + /// the same peer must coalesce into one pending response, not two. #[test] fn push_coalesces_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); @@ -1216,15 +1214,7 @@ mod tests { responses.push(1, 0xaaaa, mapped); responses.push(2, 0xbbbb, plain); - assert_eq!( - responses.pending.len(), - 1, - "the same peer under different address representations must coalesce \ - into a single pending response" - ); - assert_eq!( - responses.pending[0].token, 0xbbbb, - "the later (higher packet number) response should have been queued" - ); + assert_eq!(responses.pending.len(), 1); + assert_eq!(responses.pending[0].token, 0xbbbb); } } diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 8d3eb41f6b..105807c34c 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -531,8 +531,8 @@ mod four_tuple_tests { use super::*; - /// noq#738: two `FourTuple`s differing only by IPv4-mapped-IPv6 vs plain-IPv4 - /// representation must compare (and hash) equal. + /// noq#738: mapped vs. plain IPv4 representations of the same peer must be + /// equal (and hash equal). #[test] fn four_tuple_eq_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); @@ -545,11 +545,9 @@ mod four_tuple_tests { assert_eq!(set.len(), 1); } - /// Same as [`four_tuple_eq_ignores_mapped_v4_representation`], but for - /// `local_ip` instead of `remote` -- this is the half of the fix that the - /// original bug report actually depends on (`open_path()`'s `local_ip` - /// argument), and unlike `remote` it's never exercised by - /// `FourTuple::from_remote` (which always sets `local_ip: None`). + /// Same as [`four_tuple_eq_ignores_mapped_v4_representation`] for `local_ip` -- + /// the half `open_path()`'s bug report actually depends on, and never + /// exercised by `FourTuple::from_remote` (`local_ip: None`). #[test] fn four_tuple_eq_ignores_mapped_v4_representation_for_local_ip() { let remote = "9.9.9.9:443".parse().unwrap(); @@ -563,11 +561,9 @@ mod four_tuple_tests { assert_eq!(set.len(), 1); } - /// noq#738 fix regression: two link-local `FourTuple`s that differ only in - /// `scope_id` (i.e. reachable via different network interfaces) must NOT - /// compare equal, even though their canonicalized IP is identical. An earlier - /// version of this fix dropped `scope_id` from the comparison entirely, which - /// silently collapsed distinct interfaces into a single path. + /// noq#738 regression: link-local `FourTuple`s differing only in `scope_id` + /// (different interfaces) must NOT compare equal -- an earlier fix draft + /// dropped `scope_id` entirely, collapsing distinct interfaces into one path. #[test] fn four_tuple_eq_preserves_link_local_scope_id() { let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); @@ -580,15 +576,9 @@ mod four_tuple_tests { assert_eq!(set.len(), 2); } - /// The inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a - /// *global* (non-link-local, non-multicast) IPv6 address, `FourTuple::new()` - /// deliberately zeroes `scope_id` before storing (it's only meaningful for - /// link-local/multicast scopes) -- so by the time `canonical_remote()` runs, a - /// global address's `scope_id` is already 0 regardless of what was on the - /// input. This locks in that zeroing: two `FourTuple`s built from the same - /// global address but with different (bogus/leftover) scope_ids on the input - /// must still compare equal. If `FourTuple::new()` ever stopped zeroing - /// scope_id for global addresses, this would start failing. + /// Inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a *global* + /// IPv6 address, `FourTuple::new()` already zeroes `scope_id` on construction, + /// so bogus input scope_ids must still compare equal here. #[test] fn four_tuple_eq_zeroes_scope_id_for_global_v6() { let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); @@ -601,9 +591,8 @@ mod four_tuple_tests { assert_eq!(set.len(), 1); } - /// Mirrors [`four_tuple_eq_preserves_link_local_scope_id`] for the other half - /// of `FourTuple::new()`'s `requires_scope_id` condition (multicast, not just - /// unicast link-local). + /// Mirrors [`four_tuple_eq_preserves_link_local_scope_id`] for multicast + /// addresses. #[test] fn four_tuple_eq_preserves_multicast_scope_id() { let iface_a = FourTuple::from_remote("[ff02::1%3]:443".parse().unwrap()); @@ -616,16 +605,11 @@ mod four_tuple_tests { assert_eq!(set.len(), 2); } - /// [`FourTuple::is_probably_same_path`] duplicates the canonicalizing - /// `remote` comparison independently of the derived-then-hand-written - /// `PartialEq` (it can't just delegate to `==`, since it has its own - /// asymmetric "only compare local_ip if self has one" rule on top) -- test - /// the remote-canonicalization half directly so a future change can't fix - /// `PartialEq` and silently leave this one on a raw comparison. Both sides - /// have `local_ip: None` here, so this doesn't exercise the asymmetric - /// `local_ip` rule itself -- see - /// [`is_probably_same_path_ignores_mapped_v4_representation_for_local_ip`] - /// for that. + /// `is_probably_same_path` re-implements the canonicalizing `remote` + /// comparison independently of `PartialEq`, so it needs its own coverage. + /// `local_ip: None` here; see + /// [`is_probably_same_path_ignores_mapped_v4_representation_for_local_ip`] for + /// the `Some` case. #[test] fn is_probably_same_path_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); @@ -634,12 +618,10 @@ mod four_tuple_tests { assert!(plain.is_probably_same_path(&mapped)); } - /// Exercises the asymmetric `local_ip` branch that - /// [`is_probably_same_path_ignores_mapped_v4_representation`] deliberately - /// leaves untested: when `self.local_ip` is `Some(..)`, - /// `is_probably_same_path` requires the full (canonicalizing) `FourTuple` - /// equality, not just a `remote` match. A representation-only difference in - /// `local_ip` must not break that. + /// The `local_ip: Some(..)` counterpart of + /// [`is_probably_same_path_ignores_mapped_v4_representation`]: a + /// representation-only difference in `local_ip` must not break the + /// full-equality branch. #[test] fn is_probably_same_path_ignores_mapped_v4_representation_for_local_ip() { let remote = "9.9.9.9:443".parse().unwrap(); @@ -649,10 +631,8 @@ mod four_tuple_tests { assert!(plain.is_probably_same_path(&mapped)); } - /// Same as above, for the `scope_id` half of the fix. `scope_id` only exists - /// on `remote: SocketAddr` (`Ipv6Addr`/`IpAddr` -- and so `local_ip` -- has no - /// such field; `"...%3".parse::()` doesn't even parse), so this - /// varies `remote`, not `local_ip`. + /// Same as above for `scope_id`. Only `remote` can vary here -- `local_ip: + /// IpAddr` has no `scope_id` field. #[test] fn is_probably_same_path_distinguishes_link_local_scope_id() { let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); From 6777b8dd630eab3556f2aaf64f5c6de776d1057e Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 18:46:24 -0500 Subject: [PATCH 12/20] test(proto): drop noq#738 regression test that doesn't reproduce the bug @matheus23 pointed out that open_path_with_explicit_local_ip (added in an earlier revision of this branch) uses two structurally distinct IPv6 addresses for its two client interfaces, so it never exercises the actual mapped-vs-plain-IPv4 representation mismatch #738 is about, and does not fail on unpatched main. Removing it rather than leaving a test that doesn't test what it claims to. --- noq-proto/src/tests/multipath.rs | 73 +------------------------------- 1 file changed, 1 insertion(+), 72 deletions(-) diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index ef773fbf0a..2b00fd542c 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1,6 +1,6 @@ //! Tests for multipath -use std::net::{Ipv6Addr, SocketAddr}; +use std::net::SocketAddr; use std::num::NonZeroU32; use std::sync::Arc; use std::time::Duration; @@ -2293,74 +2293,3 @@ fn regression_discarded_path_stats_are_up_to_date() -> TestResult { Ok(()) } - -/// Regression test for issue #738. -/// -/// When a client opens a new path with an explicit `local_ip` set in the -/// [`FourTuple`], the path should validate successfully. On real devices the -/// `PATH_RESPONSE` was never matched to the outstanding `PATH_CHALLENGE` on -/// such paths, causing them to be abandoned with -/// [`PathAbandonReason::ValidationFailed`]. -/// -/// This test sets up a routing table where the client has a second interface -/// that can reach the *same* server address as the initial connection, and -/// opens a path on that second interface with an explicit `local_ip`. -/// -/// See -#[test] -fn open_path_with_explicit_local_ip() -> TestResult { - let _guard = subscribe(); - let mut pair = ConnPair::builder().enable_multipath().connect(); - - // Set up routing with a second client interface that can reach the same - // server as the first interface. Both server routes point to the same - // server address but link to different client interfaces, so the server - // can respond on either client interface. - let first_client_addr = pair.routes.as_basic().client_addr; - let server_addr = pair.routes.as_basic().server_addr; - let second_client_addr = { - let mut addr = first_client_addr; - if let SocketAddr::V6(v6) = &mut addr { - let s = v6.ip().segments(); - v6.set_ip(Ipv6Addr::new( - s[0], - s[1], - s[2], - s[3], - s[4], - s[5], - s[6], - s[7] + 1, - )); - } - addr - }; - - // `ManyToManyRouting::from_routes` rejects duplicate interface addresses - // (added by #721, after this test was originally written), which trips on - // the same `server_addr` appearing twice. `add_client_route`/`add_server_route` - // don't have that check, so build the same routing table incrementally instead. - let mut routing = ManyToManyRouting::simple_symmetric([first_client_addr], [server_addr]); - routing.add_client_route(second_client_addr, 0); - routing.add_server_route(server_addr, 1); - pair.routes = routing.into(); - - // Open a path with an explicit local_ip, targeting the same server as - // the initial connection (path 0). - let new_path = FourTuple::new(server_addr, Some(second_client_addr.ip())); - let path_id = pair.open_path(Client, new_path, PathStatus::Available)?; - pair.drive(); - - // The path should be established on both sides, not abandoned with - // ValidationFailed. - assert_matches!( - pair.poll(Client), - Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id - ); - assert_matches!( - pair.poll(Server), - Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id - ); - - Ok(()) -} From 66434c066950656e6fd30c5aa043d18d1d1ce0b6 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 18:46:31 -0500 Subject: [PATCH 13/20] fix(proto): stop overriding FourTuple's PartialEq/Eq/Hash @matheus23 flagged that changing FourTuple's own structural equality has far-reaching effects not evident from the diff alone -- it's public API, used directly as a HashMap/HashSet key in a few other places (e.g. the noq crate's Endpoint routing table), and this crate's downstream consumers (e.g. iroh) may rely on it meaning byte-for-byte equality. Keep #[derive(Hash, Eq, PartialEq, Copy, Clone)] as on main, and keep using is_same_remote()/is_same_local_ip() only at the specific comparison call sites (early_discard_packet, PATH_CHALLENGE-on-active-path detection, OBSERVED_ADDR matching, local_ip/peer migration detection, PathResponses::push) that actually need to ignore the mapped-vs-plain-IPv4 representation difference -- unchanged from the previous revision of this branch and already verified against the reported failure on real Android hardware. Rewrite the unit tests accordingly: they now assert on is_same_remote()/is_same_local_ip() directly instead of on FourTuple's own == and HashSet dedup behavior. --- noq-proto/src/lib.rs | 118 +++++++++++++++++-------------------------- 1 file changed, 46 insertions(+), 72 deletions(-) diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 105807c34c..0ce61fb16b 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -368,17 +368,21 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. /// -/// noq#738: `PartialEq`/`Eq`/`Hash` are hand-written (see below) to canonicalize -/// IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) down to plain IPv4 *only for -/// comparison/hashing purposes* — the `remote`/`local_ip` fields themselves are left -/// exactly as constructed. Canonicalizing the stored value instead (i.e. in `new()`) -/// was tried and reverted: it changes the address family of what gets handed to the -/// OS for sending (`Transmit::destination`), which is unverified on non-Linux -/// platforms (this crate's CI is Linux-only) and broke IPv6-vs-IPv4 autodetection in -/// the `noq` crate's `normalize_network_path`. Comparing canonically without mutating -/// storage gets the same "fix it once, every downstream comparison benefits" property -/// without either risk. -#[derive(Copy, Clone)] +/// noq#738: [`Self::is_same_remote`]/[`Self::is_same_local_ip`] canonicalize +/// IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) down to plain IPv4 *only for the +/// comparison itself* — the `remote`/`local_ip` fields themselves are left exactly +/// as constructed, and `PartialEq`/`Eq`/`Hash` stay derived (plain structural +/// equality). Two things were deliberately avoided: +/// - Canonicalizing the stored value instead (i.e. in `new()`): changes the address +/// family of what gets handed to the OS for sending (`Transmit::destination`), +/// which is unverified on non-Linux platforms (this crate's CI is Linux-only) and +/// broke IPv6-vs-IPv4 autodetection in the `noq` crate's `normalize_network_path`. +/// - Overriding `PartialEq`/`Eq`/`Hash` on the type itself: `FourTuple` is public +/// API, used directly as a `HashMap`/`HashSet` key in a few other places (e.g. the +/// `noq` crate's `Endpoint` routing table); changing what "equal" means there is a +/// much bigger, less obviously safe change than fixing the handful of call sites +/// that actually need to ignore the mapped-vs-plain distinction. +#[derive(Hash, Eq, PartialEq, Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. remote: SocketAddr, @@ -482,21 +486,6 @@ impl FourTuple { } } -impl PartialEq for FourTuple { - fn eq(&self, other: &Self) -> bool { - self.is_same_remote(other) && self.is_same_local_ip(other) - } -} - -impl Eq for FourTuple {} - -impl std::hash::Hash for FourTuple { - fn hash(&self, state: &mut H) { - self.canonical_remote().hash(state); - self.canonical_local_ip().hash(state); - } -} - impl fmt::Display for FourTuple { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("(local: ")?; @@ -527,82 +516,67 @@ impl From for FourTuple { #[cfg(test)] mod four_tuple_tests { - use std::collections::HashSet; - use super::*; - /// noq#738: mapped vs. plain IPv4 representations of the same peer must be - /// equal (and hash equal). + /// noq#738: mapped vs. plain IPv4 representations of the same peer must compare + /// equal via [`FourTuple::is_same_remote`] (structural `PartialEq` stays + /// derived and unaware of this -- see the type-level docs above). #[test] - fn four_tuple_eq_ignores_mapped_v4_representation() { + fn is_same_remote_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); - assert_eq!(mapped, plain); - - let mut set = HashSet::new(); - set.insert(mapped); - set.insert(plain); - assert_eq!(set.len(), 1); + assert!(mapped.is_same_remote(&plain)); + assert!(plain.is_same_remote(&mapped)); + assert_ne!(mapped, plain, "structural PartialEq must stay derived"); } - /// Same as [`four_tuple_eq_ignores_mapped_v4_representation`] for `local_ip` -- - /// the half `open_path()`'s bug report actually depends on, and never - /// exercised by `FourTuple::from_remote` (`local_ip: None`). + /// Same as [`is_same_remote_ignores_mapped_v4_representation`] for + /// [`FourTuple::is_same_local_ip`] -- the half `open_path()`'s bug report + /// actually depends on, and never exercised by `FourTuple::from_remote` + /// (`local_ip: None`). #[test] - fn four_tuple_eq_ignores_mapped_v4_representation_for_local_ip() { + fn is_same_local_ip_ignores_mapped_v4_representation() { let remote = "9.9.9.9:443".parse().unwrap(); let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); - assert_eq!(mapped, plain); - - let mut set = HashSet::new(); - set.insert(mapped); - set.insert(plain); - assert_eq!(set.len(), 1); + assert!(mapped.is_same_local_ip(&plain)); + assert!(plain.is_same_local_ip(&mapped)); + assert_ne!(mapped, plain, "structural PartialEq must stay derived"); } /// noq#738 regression: link-local `FourTuple`s differing only in `scope_id` - /// (different interfaces) must NOT compare equal -- an earlier fix draft - /// dropped `scope_id` entirely, collapsing distinct interfaces into one path. + /// (different interfaces) must NOT compare same via `is_same_remote` -- an + /// earlier fix draft dropped `scope_id` entirely, collapsing distinct + /// interfaces into one path. #[test] - fn four_tuple_eq_preserves_link_local_scope_id() { + fn is_same_remote_preserves_link_local_scope_id() { let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); - assert_ne!(iface_a, iface_b); - - let mut set = HashSet::new(); - set.insert(iface_a); - set.insert(iface_b); - assert_eq!(set.len(), 2); + assert!(!iface_a.is_same_remote(&iface_b)); + assert!(!iface_b.is_same_remote(&iface_a)); } - /// Inverse of [`four_tuple_eq_preserves_link_local_scope_id`]: for a *global* + /// Inverse of [`is_same_remote_preserves_link_local_scope_id`]: for a *global* /// IPv6 address, `FourTuple::new()` already zeroes `scope_id` on construction, - /// so bogus input scope_ids must still compare equal here. + /// so bogus input scope_ids must still compare equal here (both via + /// `is_same_remote` and structural `PartialEq`, since `new()` normalizes the + /// stored value in this case). #[test] - fn four_tuple_eq_zeroes_scope_id_for_global_v6() { + fn is_same_remote_zeroes_scope_id_for_global_v6() { let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); let b = FourTuple::from_remote("[2001:db8::1%5]:443".parse().unwrap()); + assert!(a.is_same_remote(&b)); assert_eq!(a, b); - - let mut set = HashSet::new(); - set.insert(a); - set.insert(b); - assert_eq!(set.len(), 1); } - /// Mirrors [`four_tuple_eq_preserves_link_local_scope_id`] for multicast + /// Mirrors [`is_same_remote_preserves_link_local_scope_id`] for multicast /// addresses. #[test] - fn four_tuple_eq_preserves_multicast_scope_id() { + fn is_same_remote_preserves_multicast_scope_id() { let iface_a = FourTuple::from_remote("[ff02::1%3]:443".parse().unwrap()); let iface_b = FourTuple::from_remote("[ff02::1%5]:443".parse().unwrap()); - assert_ne!(iface_a, iface_b); - - let mut set = HashSet::new(); - set.insert(iface_a); - set.insert(iface_b); - assert_eq!(set.len(), 2); + assert!(!iface_a.is_same_remote(&iface_b)); + assert!(!iface_b.is_same_remote(&iface_a)); } /// `is_probably_same_path` re-implements the canonicalizing `remote` From 3efbecf68dffdacbec2fefe342a9f3617f71840b Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 18:46:38 -0500 Subject: [PATCH 14/20] fix(noq): canonicalize local_ip in normalize_network_path like remote @matheus23 asked us to identify where non-normalized addresses leak from the outside (caller/wire) to the inside instead of patching comparisons. normalize_network_path() is exactly that boundary for open_path()/ open_path_ensure(): it already canonicalizes remote via ensure_ipv6() when the connection is dual-stack, but passed local_ip through unchanged, so an application-supplied plain IPv4 local_ip (e.g. read from the OS's network interface list) could end up stored in a different representation than remote on the same FourTuple. This closes that specific inconsistency. Unlike the noq-proto-level comparison fixes (verified against the real reported failure on Android hardware with physical Wi-Fi/cellular interfaces), I could not reproduce noq#738's actual symptom with this alone in a loopback-only environment to confirm it's part of the root cause on its own -- see the new test's doc comment for what was and wasn't reproducible here. Included regardless because it fixes a real, independently-motivated inconsistency with how remote is already handled in this same function. --- noq/src/connection.rs | 26 +++++++-- noq/src/tests.rs | 119 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/noq/src/connection.rs b/noq/src/connection.rs index fd1c909555..ebc8f2e224 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -952,9 +952,23 @@ impl Connection { /// Normalizes a [`FourTuple`] against the connection's address family. /// -/// If the connection already uses IPv6 paths, the remote is canonicalised via -/// [`ensure_ipv6`]. If it uses IPv4 and the requested remote is IPv6, this returns -/// [`PathError::InvalidRemoteAddress`]. +/// If the connection already uses IPv6 paths, both `remote` and `local_ip` are +/// canonicalised via [`ensure_ipv6`]/[`IpAddr::to_ipv6_mapped`]. If it uses IPv4 and +/// the requested remote is IPv6, this returns [`PathError::InvalidRemoteAddress`]. +/// +/// noq#738: `local_ip` used to be passed through unchanged in the IPv6 branch, while +/// `remote` was canonicalised. This is the one place where addresses supplied by the +/// caller (as opposed to observed from the OS) enter [`Connection`], so keeping +/// `local_ip` in the same representation as `remote` here avoids relying on every +/// downstream comparison to canonicalize it individually. Note: unlike the +/// `noq-proto`-level comparison fixes for #738 (which were verified against the +/// actual reported failure on real Android hardware with physical Wi-Fi/cellular +/// interfaces), this specific normalization was not independently verified to fix +/// #738's reported symptom on its own -- see the doc comment on the `noq` crate's +/// `open_path_with_explicit_ipv4_local_ip_on_dualstack_socket` test for what was and +/// wasn't reproducible in a loopback-only environment. It is included because it +/// closes a real inconsistency with `remote`'s handling in this same function, +/// independent of whether it's also part of #738's root cause. fn normalize_network_path( network_path: FourTuple, conn: &proto::Connection, @@ -978,7 +992,11 @@ fn normalize_network_path( Err(PathError::InvalidRemoteAddress(remote)) } else if ipv6 { let remote = SocketAddr::V6(ensure_ipv6(remote)); - Ok(FourTuple::new(remote, network_path.local_ip())) + let local_ip = network_path.local_ip().map(|ip| match ip { + IpAddr::V4(v4) => IpAddr::V6(v4.to_ipv6_mapped()), + IpAddr::V6(_) => ip, + }); + Ok(FourTuple::new(remote, local_ip)) } else { Ok(network_path) } diff --git a/noq/src/tests.rs b/noq/src/tests.rs index adcd75682e..ab7cf12158 100755 --- a/noq/src/tests.rs +++ b/noq/src/tests.rs @@ -336,6 +336,25 @@ impl EndpointFactory { endpoint } + + /// Like [`Self::endpoint_with_config`], but for a client-only endpoint bound to + /// `addr` instead of the default IPv4 loopback address. Used by tests that need + /// a specific bind address (e.g. a dual-stack wildcard `[::]:0`) rather than + /// self-connecting IPv4 loopback endpoints. + fn client_endpoint_with_config( + &self, + addr: SocketAddr, + transport_config: TransportConfig, + ) -> Endpoint { + let transport_config = Arc::new(transport_config); + let mut roots = RootCertStore::empty(); + roots.add(self.cert.cert.der().clone()).unwrap(); + let endpoint = Endpoint::client(addr).unwrap(); + let mut client_config = ClientConfig::with_root_certificates(Arc::new(roots)).unwrap(); + client_config.transport_config(transport_config); + endpoint.set_default_client_config(client_config); + endpoint + } } #[tokio::test] @@ -1115,6 +1134,106 @@ async fn test_multipath_observed_address() { tokio::join!(server_task, client_task); } +/// Coverage for `normalize_network_path`'s `local_ip` canonicalization (noq#738). +/// +/// A client bound to a dual-stack wildcard socket (`[::]:0`) opens an additional +/// path with an explicit *plain* IPv4 `local_ip` -- standing in for an application +/// that enumerated its own network interfaces and passed one of their addresses +/// straight through, the way `open_path`'s docs describe. The path must validate, +/// and the established `Path::local_ip()` must reflect the address that was +/// actually requested (not silently fall back to a different one). +/// +/// Note this is *not* a regression test that fails on unpatched `main`: on this +/// loopback setup the mismatch this fix targets (an application-supplied plain +/// IPv4 `local_ip` vs. the representation [`noq_udp::RecvMeta::dst_ip`] reports for +/// datagrams received on a dual-stack socket, see +/// [`noq_proto::FourTuple`](proto::FourTuple)'s docs) does not by itself block path +/// validation here, for reasons not fully tracked down -- most likely because +/// `early_discard_packet`'s defending comparison only gates already-established +/// paths, not the initial `PATH_CHALLENGE`/`PATH_RESPONSE` handshake. The #738 +/// report's real symptom was only reproduced on real Android hardware with +/// physical Wi-Fi/cellular interfaces; this environment has no equivalent, so +/// this test only guards against future regressions in the normalization itself, +/// not against #738 recurring. +#[tokio::test] +async fn open_path_with_explicit_ipv4_local_ip_on_dualstack_socket() -> TestResult { + let _logging = subscribe(); + let factory = EndpointFactory::new(); + + let mut transport_config = TransportConfig::default(); + transport_config.max_concurrent_multipath_paths(2); + let server = factory.endpoint_with_config("server", transport_config.clone()); + let server_addr = server.local_addr()?; + assert_eq!( + server_addr.ip(), + IpAddr::V4(Ipv4Addr::LOCALHOST), + "test assumes the server is plain IPv4 loopback" + ); + + let server_task = async move { + let conn = server.accept().await.ok_or("closed conn?")?.await?; + conn.closed().await; + TestResult::Ok(()) + } + .instrument(info_span!("server")); + + let client = factory.client_endpoint_with_config( + SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 0), + transport_config, + ); + + let client_task = async move { + let conn = client.connect(server_addr, "localhost")?.await?; + assert!(conn.is_multipath_enabled()); + // small synchronization step necessary to allow the server to set remote CIDs, + // see the same comment in `test_multipath_observed_address` above. + tokio::time::sleep(Duration::from_millis(200)).await; + + // A second, distinct loopback address standing in for a real second network + // interface's address. Deliberately plain IPv4, not pre-mapped -- the same + // representation an app would get from enumerating its own interfaces. + let second_local_ip = IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)); + let open = async { + loop { + match conn + .open_path( + FourTuple::new(server_addr, Some(second_local_ip)), + PathStatus::Available, + ) + .await + { + Ok(path) => break Ok(path), + Err(proto::PathError::RemoteCidsExhausted) => { + tokio::time::sleep(Duration::from_millis(20)).await; + } + Err(err) => break Err(err), + } + } + }; + let path = tokio::time::timeout(Duration::from_secs(10), open) + .await + .map_err(|_| "path with explicit local_ip timed out instead of establishing")??; + // The client is dual-stack, so the established path's remote is normalized + // to IPv4-mapped-IPv6 (see `normalize_network_path`); compare canonically. + let remote = path.remote_address()?; + assert_eq!(remote.ip().to_canonical(), server_addr.ip()); + assert_eq!(remote.port(), server_addr.port()); + assert_eq!( + path.local_ip()?.map(|ip| ip.to_canonical()), + Some(second_local_ip), + "path claims to use a different local_ip than requested -- \ + the source address selection silently fell back instead of erroring" + ); + TestResult::Ok(()) + } + .instrument(info_span!("client")); + + let (server_res, client_res) = tokio::join!(server_task, client_task); + server_res?; + client_res?; + Ok(()) +} + #[tokio::test] async fn on_closed() { let _guard = subscribe(); From 8919db5ed1cadd44371f5a9a2b0f75c533eb9cc7 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 19:26:57 -0500 Subject: [PATCH 15/20] fix(proto): normalize local_ip at Connection entry boundaries Following up on Option 1: instead of the previous per-comparison-site is_same_local_ip() hack, normalize local_ip eagerly at every point a FourTuple enters noq-proto::Connection's owned state from outside -- Connection::new (the initial path), open_path/open_path_ensure (the public proto-level APIs, so embedders bypassing the noq wrapper crate get the invariant too), incoming datagram handling, and the server's first-packet handling. This means every local_ip comparison inside Connection can now be a plain == -- delete is_same_local_ip() and update the two call sites (early_discard_packet's migration guard and the local_ip-migration check) accordingly. remote is deliberately left untouched (is_same_remote() stays): unlike local_ip, PathData.network_path.remote also drives Transmit::destination for actual OS sends, so normalizing it here risks the same problem the very first (abandoned) attempt at this fix hit. That needs a separate, larger PathData network_path/transmit_path split, which will be proposed as an independent follow-up PR for maintainers to weigh in on rather than folded into this one. Added noq-proto/src/tests/multipath.rs coverage that opens a path via Connection::open_path/open_path_ensure directly (bypassing the noq wrapper) with a mapped-vs-plain-IPv4 local_ip and confirms it's recognized as the same path either way -- this is the part that was previously only exercised through the noq wrapper crate's own normalize_network_path, not at the noq-proto level itself. --- noq-proto/src/connection/mod.rs | 51 ++++++++++++++++++++-- noq-proto/src/lib.rs | 74 +++++++++++--------------------- noq-proto/src/tests/multipath.rs | 35 ++++++++++++++- 3 files changed, 107 insertions(+), 53 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 4419cc2f29..e5bc5ddd32 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3,7 +3,7 @@ use std::{ collections::{BTreeMap, VecDeque, btree_map}, convert::TryFrom, fmt, io, mem, - net::SocketAddr, + net::{IpAddr, SocketAddr}, num::{NonZeroU32, NonZeroUsize}, sync::Arc, }; @@ -324,6 +324,10 @@ impl Connection { side_args: SideArgs, qlog: QlogSink, ) -> Self { + let network_path = Self::normalize_network_path_local_ip_for_family( + network_path, + network_path.remote.is_ipv6(), + ); let pref_addr_cid = side_args.pref_addr_cid(); let path_validated = side_args.path_validated(); let connection_side = ConnectionSide::from(side_args); @@ -537,6 +541,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result<(PathId, bool), PathError> { + let network_path = self.normalize_network_path_local_ip(network_path); let existing_open_path = self.paths.iter().find(|(id, path)| { network_path.is_probably_same_path(&path.data.network_path) && !self.abandoned_paths.contains(id) @@ -558,6 +563,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result { + let network_path = self.normalize_network_path_local_ip(network_path); let Some(max_path_id) = self.max_path_id() else { return Err(PathError::MultipathNotNegotiated); }; @@ -2220,6 +2226,11 @@ impl Connection { first_decode, remaining, }) => { + // `network_path` comes from the endpoint's UDP receive metadata (`RecvMeta` in the + // `noq` crate), so this is a `Connection` entry boundary for OS-observed local + // addresses. Normalize `local_ip` before it can update `PathData.network_path`; + // `remote` is intentionally preserved because it also drives `Transmit::destination`. + let network_path = self.normalize_network_path_local_ip(network_path); let span = trace_span!("pkt", %path_id); let _guard = span.enter(); @@ -2339,7 +2350,7 @@ impl Connection { if known_path.network_path.local_ip.is_some() && network_path.local_ip.is_some() - && !network_path.is_same_local_ip(&known_path.network_path) + && network_path.local_ip != known_path.network_path.local_ip && !local_ip_may_migrate { trace!( @@ -3912,6 +3923,7 @@ impl Connection { packet: InitialPacket, remaining: Option, ) -> Result<(), ConnectionError> { + let network_path = self.normalize_network_path_local_ip(network_path); let span = trace_span!("first recv"); let _guard = span.enter(); debug_assert!(self.side.is_server()); @@ -5567,7 +5579,7 @@ impl Connection { // above, so comparing against `network_path` itself covers the // `new_local_ip` side. if path_data.network_path.local_ip.is_some() - && !path_data.network_path.is_same_local_ip(&network_path) + && path_data.network_path.local_ip != network_path.local_ip { debug!( %path_id, @@ -7077,6 +7089,39 @@ impl Connection { .any(|p| p.data.network_path.remote.is_ipv6()) } + /// Normalizes caller- or OS-supplied local IPs at `Connection` entry boundaries. + /// + /// noq#738 / PR #784: `PathData.network_path.local_ip` is now normalized before it + /// enters connection-owned state, so plain `.local_ip ==` comparisons are safe inside + /// `noq-proto`. `remote` is deliberately not normalized here because + /// `PathData.network_path.remote` also drives `Transmit::destination` for actual OS sends; + /// fixing that needs the follow-up PathData network_path/transmit_path split discussed in + /// the issue/PR. + fn normalize_network_path_local_ip(&self, network_path: FourTuple) -> FourTuple { + Self::normalize_network_path_local_ip_for_family(network_path, self.is_ipv6()) + } + + fn normalize_network_path_local_ip_for_family( + network_path: FourTuple, + ipv6: bool, + ) -> FourTuple { + let local_ip = network_path + .local_ip + .map(|ip| Self::normalize_local_ip(ip, ipv6)); + FourTuple::new(network_path.remote, local_ip) + } + + fn normalize_local_ip(ip: IpAddr, ipv6: bool) -> IpAddr { + if ipv6 { + match ip { + IpAddr::V4(v4) => IpAddr::V6(v4.to_ipv6_mapped()), + IpAddr::V6(_) => ip, + } + } else { + ip.to_canonical() + } + } + /// Add addresses the local endpoint considers are reachable for nat traversal. pub fn add_nat_traversal_address( &mut self, diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 0ce61fb16b..60acfd718d 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -368,20 +368,15 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. /// -/// noq#738: [`Self::is_same_remote`]/[`Self::is_same_local_ip`] canonicalize -/// IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) down to plain IPv4 *only for the -/// comparison itself* — the `remote`/`local_ip` fields themselves are left exactly -/// as constructed, and `PartialEq`/`Eq`/`Hash` stay derived (plain structural -/// equality). Two things were deliberately avoided: -/// - Canonicalizing the stored value instead (i.e. in `new()`): changes the address -/// family of what gets handed to the OS for sending (`Transmit::destination`), -/// which is unverified on non-Linux platforms (this crate's CI is Linux-only) and -/// broke IPv6-vs-IPv4 autodetection in the `noq` crate's `normalize_network_path`. -/// - Overriding `PartialEq`/`Eq`/`Hash` on the type itself: `FourTuple` is public -/// API, used directly as a `HashMap`/`HashSet` key in a few other places (e.g. the -/// `noq` crate's `Endpoint` routing table); changing what "equal" means there is a -/// much bigger, less obviously safe change than fixing the handful of call sites -/// that actually need to ignore the mapped-vs-plain distinction. +/// +/// noq#738 / PR #784: `noq-proto::Connection` normalizes `local_ip` at every entry +/// boundary before storing a `FourTuple` in connection-owned state, so plain +/// `.local_ip ==` comparisons are safe there. `remote` is not yet normalized at the +/// `noq-proto` level because `PathData.network_path.remote` also drives +/// `Transmit::destination` for actual OS sends; see the issue/PR discussion and the +/// follow-up PathData network_path/transmit_path split for the full fix. Until then, +/// connection-internal remote comparisons that need to ignore mapped-vs-plain IPv4 +/// representation differences use [`Self::is_same_remote`]. #[derive(Hash, Eq, PartialEq, Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. @@ -453,24 +448,19 @@ impl FourTuple { (ip, self.remote.port(), scope) } - /// noq#738: same rationale as [`Self::canonical_remote`], for `local_ip`. - fn canonical_local_ip(&self) -> Option { - self.local_ip.map(|ip| ip.to_canonical()) - } - - /// noq#738: whether `self` and `other` share the same remote peer, ignoring the - /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this fix. - /// Used everywhere a raw `remote == remote` comparison would otherwise bypass - /// this canonicalization (see the type-level docs above). + /// Whether `self` and `other` share the same remote peer, ignoring the + /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated noq#738. + /// + /// `local_ip` is now normalized at every `Connection` entry boundary (see + /// `Connection::normalize_network_path_local_ip`), so plain `.local_ip ==` + /// comparisons are safe there. `remote` is NOT yet normalized at the `noq-proto` + /// level because `PathData.network_path.remote` also drives `Transmit::destination` + /// for actual OS sends; see issue #738 / PR #784 and the follow-up PR for the + /// PathData network_path/transmit_path split. pub(crate) fn is_same_remote(&self, other: &Self) -> bool { self.canonical_remote() == other.canonical_remote() } - /// noq#738: same rationale as [`Self::is_same_remote`], for `local_ip`. - pub(crate) fn is_same_local_ip(&self, other: &Self) -> bool { - self.canonical_local_ip() == other.canonical_local_ip() - } - /// Returns whether we think the other address probably represents the same path /// as ours. /// @@ -482,7 +472,7 @@ impl FourTuple { /// - `a.is_probably_same_path(b)` /// - `b.is_probably_same_path(a)` pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { - self.is_same_remote(other) && (self.local_ip.is_none() || self.is_same_local_ip(other)) + self.is_same_remote(other) && (self.local_ip.is_none() || self.local_ip == other.local_ip) } } @@ -530,20 +520,6 @@ mod four_tuple_tests { assert_ne!(mapped, plain, "structural PartialEq must stay derived"); } - /// Same as [`is_same_remote_ignores_mapped_v4_representation`] for - /// [`FourTuple::is_same_local_ip`] -- the half `open_path()`'s bug report - /// actually depends on, and never exercised by `FourTuple::from_remote` - /// (`local_ip: None`). - #[test] - fn is_same_local_ip_ignores_mapped_v4_representation() { - let remote = "9.9.9.9:443".parse().unwrap(); - let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); - let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); - assert!(mapped.is_same_local_ip(&plain)); - assert!(plain.is_same_local_ip(&mapped)); - assert_ne!(mapped, plain, "structural PartialEq must stay derived"); - } - /// noq#738 regression: link-local `FourTuple`s differing only in `scope_id` /// (different interfaces) must NOT compare same via `is_same_remote` -- an /// earlier fix draft dropped `scope_id` entirely, collapsing distinct @@ -593,16 +569,16 @@ mod four_tuple_tests { } /// The `local_ip: Some(..)` counterpart of - /// [`is_probably_same_path_ignores_mapped_v4_representation`]: a - /// representation-only difference in `local_ip` must not break the - /// full-equality branch. + /// [`is_probably_same_path_ignores_mapped_v4_representation`]: representation + /// differences in `local_ip` are deliberately not hidden here. `Connection` + /// normalizes local IPs at its entry boundaries before storing them. #[test] - fn is_probably_same_path_ignores_mapped_v4_representation_for_local_ip() { + fn is_probably_same_path_uses_structural_local_ip_equality() { let remote = "9.9.9.9:443".parse().unwrap(); let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); - assert!(mapped.is_probably_same_path(&plain)); - assert!(plain.is_probably_same_path(&mapped)); + assert!(!mapped.is_probably_same_path(&plain)); + assert!(!plain.is_probably_same_path(&mapped)); } /// Same as above for `scope_id`. Only `remote` can vary here -- `local_ip: diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 2b00fd542c..c7503bc47b 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1,6 +1,6 @@ //! Tests for multipath -use std::net::SocketAddr; +use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::num::NonZeroU32; use std::sync::Arc; use std::time::Duration; @@ -384,6 +384,39 @@ fn open_path() -> TestResult { Ok(()) } +/// Confirms the proto-level `Connection::open_path` and `open_path_ensure` APIs +/// normalize `local_ip` before matching or storing paths, even when callers bypass +/// the `noq` wrapper crate. +#[test] +fn open_path_normalizes_local_ip_for_dual_stack_connection() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + let server_addr = pair.routes.public_server_addr(); + let plain_local_ip = Ipv4Addr::new(192, 0, 2, 1); + let mapped_local_ip = IpAddr::V6(plain_local_ip.to_ipv6_mapped()); + + let opened_with_mapped = FourTuple::new(server_addr, Some(mapped_local_ip)); + let path_id = pair.open_path(Client, opened_with_mapped, PathStatus::Available)?; + assert_eq!( + pair.network_path(Client, path_id)?.local_ip(), + Some(mapped_local_ip) + ); + + let requested_with_plain = FourTuple::new(server_addr, Some(IpAddr::V4(plain_local_ip))); + let (same_path_id, existed) = + pair.open_path_ensure(Client, requested_with_plain, PathStatus::Available)?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.local_ip(), + Some(mapped_local_ip) + ); + + Ok(()) +} + #[test] fn open_path_key_update() -> TestResult { let _guard = subscribe(); From aa53556b95251fb51aaf39e1eaa30da0a4dd7ca5 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 21:03:58 -0500 Subject: [PATCH 16/20] fix(proto): fix is_ipv6() at connection establishment, not live path scan is_ipv6() previously re-derived the connection's address family on every call from whatever paths currently happen to exist (paths.values().any(...)). Since local_ip normalization now depends on this predicate, a connection whose only IPv6-family path gets abandoned could have is_ipv6() flip back to false mid-connection, causing already-normalized (mapped) local_ips to stop matching freshly-observed ones -- a variant of the exact bug #738 is about, self-inflicted. Fix it at Connection::new() from the initial path's remote family and never recompute it afterward. This also collapses two independently derived notions of the connection's address family that existed in this codebase (noq-proto's own .any()-based one, and the noq wrapper crate's separate .next()-based one in normalize_network_path) into a single source of truth: the noq wrapper now just calls conn.is_ipv6() directly. Known limitation, documented on the new field: this does not adapt to Endpoint::rebind() changing the underlying socket's family mid-connection -- noq-proto::Connection currently has no signal for that at all (its own ConnectionEventInner has no rebind concept, and the noq wrapper's ConnectionEvent::Rebind never reaches here). Wiring that up is a real gap but is out of scope for this fix; flagging it for maintainers rather than silently leaving it unmentioned. Also correct the doc comment on local_ip normalization: it previously claimed this doesn't change what's handed to the OS, which is wrong -- local_ip becomes Transmit::src_ip, and noq-udp's unix backend sends a different control message (IP_PKTINFO vs IPV6_PKTINFO) depending on whether it's IpAddr::V4 or IpAddr::V6. Reframed as: this is the correct behavior for a dual-stack socket, and the cmsg-family mismatch this fixes is a plausible (unverified without real multi-interface hardware) explanation for #738's actual root cause, not just a comparison-time cosmetic issue. Add the regression test divagant-martian asked for: a ManyToManyRouting setup where the client's local interface is only routable in its IPv4-mapped-IPv6 form, opening a path with the plain-IPv4 representation of that same address. Verified this fails on the pre-fix code (the test harness's own routing simulation drops every packet with 'no route from client to server', since the source address representation doesn't match any route) and passes with the fix. --- noq-proto/src/connection/mod.rs | 52 ++++++++++++++++++-------- noq-proto/src/connection/paths.rs | 10 +++-- noq-proto/src/tests/multipath.rs | 61 +++++++++++++++++++++++++++++++ noq/src/connection.rs | 15 +------- 4 files changed, 105 insertions(+), 33 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index e5bc5ddd32..3c8cf6b526 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -171,6 +171,15 @@ pub struct Connection { /// deterministically select the next PathId to send on. // TODO(flub): well does it really? But deterministic is nice for now. paths: BTreeMap, + /// Fixed at connection establishment from the initial path's remote family. + /// + /// This does NOT adapt if `Endpoint::rebind()` later changes the underlying socket's + /// address family mid-connection: `noq-proto::Connection` currently has no signal for + /// that. The proto-level [`ConnectionEventInner`] only carries datagrams and issued + /// connection identifiers, while the `noq` wrapper's own `ConnectionEvent::Rebind` + /// never reaches here. Extending that signal is out of scope for this fix and should be + /// flagged for maintainers. + established_ipv6: bool, /// Counter to uniquely identify every [`PathData`] created in this connection. /// /// Each [`PathData`] gets a [`PathData::generation`] that is unique among all @@ -324,10 +333,9 @@ impl Connection { side_args: SideArgs, qlog: QlogSink, ) -> Self { - let network_path = Self::normalize_network_path_local_ip_for_family( - network_path, - network_path.remote.is_ipv6(), - ); + let established_ipv6 = network_path.remote.is_ipv6(); + let network_path = + Self::normalize_network_path_local_ip_for_family(network_path, established_ipv6); let pref_addr_cid = side_args.pref_addr_cid(); let path_validated = side_args.path_validated(); let connection_side = ConnectionSide::from(side_args); @@ -370,6 +378,7 @@ impl Connection { handshake_cid: local_cid, remote_handshake_cid: remote_cid, local_cid_state, + established_ipv6: false, paths: BTreeMap::from_iter([( PathId::ZERO, PathState { @@ -434,6 +443,7 @@ impl Connection { n0_nat_traversal: Default::default(), qlog, }; + this.set_socket_family(established_ipv6); if path_validated { this.on_path_validated(PathId::ZERO); } @@ -7079,24 +7089,36 @@ impl Connection { } } - /// Returns whether this connection has a socket that supports IPv6. + /// Returns whether this connection was established on an IPv6-family socket. + pub fn is_ipv6(&self) -> bool { + self.established_ipv6 + } + + /// Sets the fixed socket family captured when the connection is established. /// - /// TODO(matheus23): This is related to noq endpoint state's `ipv6` bool. We should move that - /// info here instead of trying to hack around not knowing it exactly. - pub(crate) fn is_ipv6(&self) -> bool { - self.paths - .values() - .any(|p| p.data.network_path.remote.is_ipv6()) + /// This has the same rebind limitation documented on [`Self::established_ipv6`]. + fn set_socket_family(&mut self, ipv6: bool) { + self.established_ipv6 = ipv6; } /// Normalizes caller- or OS-supplied local IPs at `Connection` entry boundaries. /// /// noq#738 / PR #784: `PathData.network_path.local_ip` is now normalized before it /// enters connection-owned state, so plain `.local_ip ==` comparisons are safe inside - /// `noq-proto`. `remote` is deliberately not normalized here because - /// `PathData.network_path.remote` also drives `Transmit::destination` for actual OS sends; - /// fixing that needs the follow-up PathData network_path/transmit_path split discussed in - /// the issue/PR. + /// `noq-proto`. This normalization is intentionally based on the connection's + /// established socket family: on a dual-stack socket, a plain IPv4 `local_ip` is mapped + /// to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is folded to plain IPv4. + /// + /// This does change what is handed to the OS. `PathData.network_path.local_ip` becomes + /// `Transmit::src_ip`, and the UDP backend sends different control messages for + /// `IpAddr::V4` and `IpAddr::V6` (`IP_PKTINFO` vs `IPV6_PKTINFO` on Unix). That is the + /// intended behavior for a dual-stack socket: sending IPv4 packet info while the + /// destination sockaddr is IPv6-family is a plausible reason the OS would ignore or + /// mishandle source selection in noq#738, though this root-cause hypothesis has not + /// been verified on real multi-interface hardware. `remote` is deliberately not + /// normalized here because `PathData.network_path.remote` also drives + /// `Transmit::destination`; fixing that needs the follow-up PathData + /// network_path/transmit_path split discussed in the issue/PR. fn normalize_network_path_local_ip(&self, network_path: FourTuple) -> FourTuple { Self::normalize_network_path_local_ip_for_family(network_path, self.is_ipv6()) } diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index c82f0912c7..65048b8fb1 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -894,10 +894,12 @@ impl PathResponses { pub(crate) fn pop_off_path(&mut self, network_path: FourTuple) -> Option<(u64, FourTuple)> { let response = *self.pending.last()?; - // We use an exact comparison here, because once we've received for the first time, - // we really should either already have a local_ip, or we will never get one - // (because our OS doesn't support it). And even if we get it wrong we are only - // slightly less efficient and would not include other on-path data in the packet. + // We use an exact comparison here. By the time connection-owned path state and + // received PATH_CHALLENGE metadata reach this queue, `Connection` has normalized + // `local_ip` to the established socket family. `push` still coalesces + // mapped-vs-plain `remote` representations, but exact matching here remains the + // right on-path/off-path split because different `local_ip`s can be different + // interfaces. if response.network_path == network_path { // We don't bother searching further because we expect that the on-path response will // get drained in the immediate future by a call to `pop_on_path` diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index c7503bc47b..ae7da23be2 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -417,6 +417,67 @@ fn open_path_normalizes_local_ip_for_dual_stack_connection() -> TestResult { Ok(()) } +#[test] +fn connection_socket_family_is_fixed_at_establishment() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + assert!(pair.conn(Client).is_ipv6()); + + let server_addr_v4 = "192.0.2.2:4433".parse()?; + let client_ip_v4 = Ipv4Addr::new(192, 0, 2, 1); + let path_id = pair.open_path( + Client, + FourTuple::new(server_addr_v4, Some(IpAddr::V4(client_ip_v4))), + PathStatus::Available, + )?; + + assert!(pair.conn(Client).is_ipv6()); + assert_eq!( + pair.network_path(Client, path_id)?.local_ip(), + Some(IpAddr::V6(client_ip_v4.to_ipv6_mapped())) + ); + + Ok(()) +} + +#[test] +fn open_path_with_plain_local_ip_validates_on_mapped_ipv4_route() -> TestResult { + let _guard = subscribe(); + let client_addr_0 = "[::ffff:1.1.1.0]:44433".parse::()?; + let server_addr_0 = "[::ffff:2.2.2.0]:4433".parse::()?; + let client_addr_1 = "[::ffff:1.1.1.1]:44433".parse::()?; + let server_addr_1 = "[::ffff:2.2.2.1]:4433".parse::()?; + let mut pair = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .with_routes(ManyToManyRouting::from_routes( + [(client_addr_0, 0), (client_addr_1, 1)], + [(server_addr_0, 0), (server_addr_1, 1)], + )) + .connect(); + + let client_ip_plain = client_addr_1.ip().to_canonical(); + let path_id = pair.open_path( + Client, + FourTuple::new(server_addr_1, Some(client_ip_plain)), + PathStatus::Available, + )?; + + pair.drive(); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + + Ok(()) +} + #[test] fn open_path_key_update() -> TestResult { let _guard = subscribe(); diff --git a/noq/src/connection.rs b/noq/src/connection.rs index ebc8f2e224..37f3b1d1e4 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -973,20 +973,7 @@ fn normalize_network_path( network_path: FourTuple, conn: &proto::Connection, ) -> Result { - // If endpoint::State::ipv6 is true we want to keep all our IP addresses as IPv6. - // If not, we do not support IPv6. We can not access endpoint::State from here - // however, but either all our paths use an IPv6 address, or all our paths use an - // IPv4 address. So we can use that information. - let ipv6 = conn - .paths() - .iter() - .filter_map(|id| { - conn.network_path(*id) - .map(|addrs| addrs.remote().is_ipv6()) - .ok() - }) - .next() - .unwrap_or_default(); + let ipv6 = conn.is_ipv6(); let remote = network_path.remote(); if remote.is_ipv6() && !ipv6 { Err(PathError::InvalidRemoteAddress(remote)) From 7ad9319aa0a5f1faf685ff9f615d5cc5c96e1e0a Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 21:59:54 -0500 Subject: [PATCH 17/20] docs(proto): fix rustdoc issues found by actually running cargo make's dev-flow I had only run a narrower cargo test/clippy subset before, not the project's actual Makefile.toml dev-flow (format-check, check, clippy, doc, test, proptests-extralight, all workspace-wide with --all-features). Running the real thing surfaced two genuine issues the narrower checks missed: - FourTuple's public type docs linked to Self::is_same_remote, which is private -- this resolves under --document-private-items (what I'd tested with) but is a broken intra-doc link in a normal doc build (e.g. docs.rs). Replaced the doc link with plain text, and while here, updated the stale reference to a hypothetical 'PathData network_path/transmit_path split' to instead point at noq#787, which is the actual follow-up that now exists. - normalize_network_path()'s doc comment linked to IpAddr::to_ipv6_mapped, which doesn't exist -- that method is on Ipv4Addr, not IpAddr. Fixed the link target. Also re-ran cargo fmt with this project's actual rustfmt config (comment_width=100, wrap_comments=true, from Makefile.toml) instead of plain defaults, which reflowed one over-width comment line. --- noq-proto/src/connection/mod.rs | 3 ++- noq-proto/src/lib.rs | 8 +++++--- noq/src/connection.rs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 3c8cf6b526..c7e1b9a313 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2239,7 +2239,8 @@ impl Connection { // `network_path` comes from the endpoint's UDP receive metadata (`RecvMeta` in the // `noq` crate), so this is a `Connection` entry boundary for OS-observed local // addresses. Normalize `local_ip` before it can update `PathData.network_path`; - // `remote` is intentionally preserved because it also drives `Transmit::destination`. + // `remote` is intentionally preserved because it also drives + // `Transmit::destination`. let network_path = self.normalize_network_path_local_ip(network_path); let span = trace_span!("pkt", %path_id); let _guard = span.enter(); diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 60acfd718d..c05b939a53 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -373,10 +373,12 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// boundary before storing a `FourTuple` in connection-owned state, so plain /// `.local_ip ==` comparisons are safe there. `remote` is not yet normalized at the /// `noq-proto` level because `PathData.network_path.remote` also drives -/// `Transmit::destination` for actual OS sends; see the issue/PR discussion and the -/// follow-up PathData network_path/transmit_path split for the full fix. Until then, +/// `Transmit::destination` for actual OS sends; see the issue/PR discussion, and +/// `noq#787` for an alternative that also normalizes `remote` at the connection's +/// established socket family. Until this PR's scope is extended (or superseded), /// connection-internal remote comparisons that need to ignore mapped-vs-plain IPv4 -/// representation differences use [`Self::is_same_remote`]. +/// representation differences use `Self::is_same_remote` (private, not linked here +/// since it would produce a broken intra-doc link in the published docs). #[derive(Hash, Eq, PartialEq, Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. diff --git a/noq/src/connection.rs b/noq/src/connection.rs index 37f3b1d1e4..c57a301861 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -953,7 +953,7 @@ impl Connection { /// Normalizes a [`FourTuple`] against the connection's address family. /// /// If the connection already uses IPv6 paths, both `remote` and `local_ip` are -/// canonicalised via [`ensure_ipv6`]/[`IpAddr::to_ipv6_mapped`]. If it uses IPv4 and +/// canonicalised via [`ensure_ipv6`]/[`std::net::Ipv4Addr::to_ipv6_mapped`]. If it uses IPv4 and /// the requested remote is IPv6, this returns [`PathError::InvalidRemoteAddress`]. /// /// noq#738: `local_ip` used to be passed through unchanged in the IPv6 branch, while From d42d40e1638566cdcae8711967bbdf5ca65b7830 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 23:03:59 -0500 Subject: [PATCH 18/20] chore(proto): remove comment divagant-martian flagged as removable The explanatory comment about why comparing against network_path itself covers the new_local_ip side was flagged 'fine to remove' in review; it never got dropped in the subsequent rewrites of this function. --- noq-proto/src/connection/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index c7e1b9a313..6dac5016bd 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -5586,9 +5586,6 @@ impl Connection { && let Some(new_local_ip) = network_path.local_ip { let path_data = self.path_data_mut(path_id); - // `network_path.local_ip` is `Some(new_local_ip)` per the `let Some` guard - // above, so comparing against `network_path` itself covers the - // `new_local_ip` side. if path_data.network_path.local_ip.is_some() && path_data.network_path.local_ip != network_path.local_ip { From 2dea7349f136f8c4ff348923e5820aeb9d28054f Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Mon, 10 Aug 2026 23:27:03 -0500 Subject: [PATCH 19/20] docs(proto): match this repo's issue-reference comment style Compared against how the pre-existing codebase actually references issues in comments (e.g. 'PATH_ABANDON on the abandoned path itself when no other path exists (#509).', 'Recover storage from these by compacting (#700)') -- the convention is a bare '(#NNN)' at the end of the relevant sentence, not a 'noqNNN:' prefix at the start. Reworded every doc/comment this PR chain added that used the latter style to match. Also fixed two doc comments that still referenced a hypothetical 'PathData network_path/transmit_path split' follow-up instead of the actual #787 that now exists, and a stray duplicated blank doc line. --- noq-proto/src/connection/mod.rs | 17 ++++++++-------- noq-proto/src/connection/paths.rs | 2 +- noq-proto/src/lib.rs | 32 +++++++++++++++---------------- noq/src/connection.rs | 4 ++-- noq/src/tests.rs | 2 +- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 6dac5016bd..47704a861b 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -7101,22 +7101,23 @@ impl Connection { /// Normalizes caller- or OS-supplied local IPs at `Connection` entry boundaries. /// - /// noq#738 / PR #784: `PathData.network_path.local_ip` is now normalized before it - /// enters connection-owned state, so plain `.local_ip ==` comparisons are safe inside - /// `noq-proto`. This normalization is intentionally based on the connection's - /// established socket family: on a dual-stack socket, a plain IPv4 `local_ip` is mapped - /// to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is folded to plain IPv4. + /// `PathData.network_path.local_ip` is now normalized before it enters + /// connection-owned state, so plain `.local_ip ==` comparisons are safe inside + /// `noq-proto` (#738, #784). This normalization is intentionally based on the + /// connection's established socket family: on a dual-stack socket, a plain IPv4 + /// `local_ip` is mapped to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is + /// folded to plain IPv4. /// /// This does change what is handed to the OS. `PathData.network_path.local_ip` becomes /// `Transmit::src_ip`, and the UDP backend sends different control messages for /// `IpAddr::V4` and `IpAddr::V6` (`IP_PKTINFO` vs `IPV6_PKTINFO` on Unix). That is the /// intended behavior for a dual-stack socket: sending IPv4 packet info while the /// destination sockaddr is IPv6-family is a plausible reason the OS would ignore or - /// mishandle source selection in noq#738, though this root-cause hypothesis has not + /// mishandle source selection for #738, though this root-cause hypothesis has not /// been verified on real multi-interface hardware. `remote` is deliberately not /// normalized here because `PathData.network_path.remote` also drives - /// `Transmit::destination`; fixing that needs the follow-up PathData - /// network_path/transmit_path split discussed in the issue/PR. + /// `Transmit::destination`; see #787 for an alternative that also normalizes `remote` + /// at the connection's established socket family. fn normalize_network_path_local_ip(&self, network_path: FourTuple) -> FourTuple { Self::normalize_network_path_local_ip_for_family(network_path, self.is_ipv6()) } diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index 65048b8fb1..b349e60f58 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -1205,7 +1205,7 @@ mod tests { assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX) } - /// noq#738 regression: `PATH_CHALLENGE`s on mapped-vs-plain-IPv4 FourTuples of + /// Regression test (#738): `PATH_CHALLENGE`s on mapped-vs-plain-IPv4 FourTuples of /// the same peer must coalesce into one pending response, not two. #[test] fn push_coalesces_mapped_v4_representation() { diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index c05b939a53..36a18f2089 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -368,14 +368,12 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. /// -/// -/// noq#738 / PR #784: `noq-proto::Connection` normalizes `local_ip` at every entry -/// boundary before storing a `FourTuple` in connection-owned state, so plain -/// `.local_ip ==` comparisons are safe there. `remote` is not yet normalized at the -/// `noq-proto` level because `PathData.network_path.remote` also drives -/// `Transmit::destination` for actual OS sends; see the issue/PR discussion, and -/// `noq#787` for an alternative that also normalizes `remote` at the connection's -/// established socket family. Until this PR's scope is extended (or superseded), +/// `noq-proto::Connection` normalizes `local_ip` at every entry boundary before storing +/// a `FourTuple` in connection-owned state, so plain `.local_ip ==` comparisons are safe +/// there (#738 / #784). `remote` is not yet normalized at the `noq-proto` level because +/// `PathData.network_path.remote` also drives `Transmit::destination` for actual OS +/// sends; see #787 for an alternative that also normalizes `remote` at the connection's +/// established socket family. Until this scope is extended (or superseded), /// connection-internal remote comparisons that need to ignore mapped-vs-plain IPv4 /// representation differences use `Self::is_same_remote` (private, not linked here /// since it would produce a broken intra-doc link in the published docs). @@ -434,8 +432,8 @@ impl FourTuple { self.local_ip } - /// noq#738: canonicalizes `remote` for comparison/hashing purposes (see the - /// type-level docs above). `to_canonical()` folds `::ffff:a.b.c.d` down to plain + /// Canonicalizes `remote` for comparison/hashing purposes (see the type-level docs + /// above, #738). `to_canonical()` folds `::ffff:a.b.c.d` down to plain /// IPv4; the scope_id is kept for addresses that stay IPv6, because [`Self::new`] /// deliberately preserves it for link-local and multicast remotes — two different /// link-local interfaces must not compare equal just because their canonicalized @@ -451,14 +449,14 @@ impl FourTuple { } /// Whether `self` and `other` share the same remote peer, ignoring the - /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated noq#738. + /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this (#738). /// /// `local_ip` is now normalized at every `Connection` entry boundary (see /// `Connection::normalize_network_path_local_ip`), so plain `.local_ip ==` /// comparisons are safe there. `remote` is NOT yet normalized at the `noq-proto` /// level because `PathData.network_path.remote` also drives `Transmit::destination` - /// for actual OS sends; see issue #738 / PR #784 and the follow-up PR for the - /// PathData network_path/transmit_path split. + /// for actual OS sends; see #784, and #787 for an alternative that also normalizes + /// `remote` at the connection's established socket family. pub(crate) fn is_same_remote(&self, other: &Self) -> bool { self.canonical_remote() == other.canonical_remote() } @@ -510,9 +508,9 @@ impl From for FourTuple { mod four_tuple_tests { use super::*; - /// noq#738: mapped vs. plain IPv4 representations of the same peer must compare - /// equal via [`FourTuple::is_same_remote`] (structural `PartialEq` stays - /// derived and unaware of this -- see the type-level docs above). + /// Mapped vs. plain IPv4 representations of the same peer must compare equal via + /// [`FourTuple::is_same_remote`] (structural `PartialEq` stays derived and unaware + /// of this -- see the type-level docs above) (#738). #[test] fn is_same_remote_ignores_mapped_v4_representation() { let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); @@ -522,7 +520,7 @@ mod four_tuple_tests { assert_ne!(mapped, plain, "structural PartialEq must stay derived"); } - /// noq#738 regression: link-local `FourTuple`s differing only in `scope_id` + /// Regression test (#738): link-local `FourTuple`s differing only in `scope_id` /// (different interfaces) must NOT compare same via `is_same_remote` -- an /// earlier fix draft dropped `scope_id` entirely, collapsing distinct /// interfaces into one path. diff --git a/noq/src/connection.rs b/noq/src/connection.rs index c57a301861..a4432ee8e0 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -956,8 +956,8 @@ impl Connection { /// canonicalised via [`ensure_ipv6`]/[`std::net::Ipv4Addr::to_ipv6_mapped`]. If it uses IPv4 and /// the requested remote is IPv6, this returns [`PathError::InvalidRemoteAddress`]. /// -/// noq#738: `local_ip` used to be passed through unchanged in the IPv6 branch, while -/// `remote` was canonicalised. This is the one place where addresses supplied by the +/// `local_ip` used to be passed through unchanged in the IPv6 branch, while `remote` +/// was canonicalised (#738). This is the one place where addresses supplied by the /// caller (as opposed to observed from the OS) enter [`Connection`], so keeping /// `local_ip` in the same representation as `remote` here avoids relying on every /// downstream comparison to canonicalize it individually. Note: unlike the diff --git a/noq/src/tests.rs b/noq/src/tests.rs index ab7cf12158..5ecb8b736a 100755 --- a/noq/src/tests.rs +++ b/noq/src/tests.rs @@ -1134,7 +1134,7 @@ async fn test_multipath_observed_address() { tokio::join!(server_task, client_task); } -/// Coverage for `normalize_network_path`'s `local_ip` canonicalization (noq#738). +/// Coverage for `normalize_network_path`'s `local_ip` canonicalization (#738). /// /// A client bound to a dual-stack wildcard socket (`[::]:0`) opens an additional /// path with an explicit *plain* IPv4 `local_ip` -- standing in for an application From db998f2dd883d15d763f8f32147f3c903ca88667 Mon Sep 17 00:00:00 2001 From: Tomoya Kawanishi Date: Tue, 11 Aug 2026 00:22:05 -0500 Subject: [PATCH 20/20] fix(proto): normalize remote at open_path, consolidating #787 into this PR Adds the remaining piece from the sibling exploration in #787: caller- supplied `remote` addresses passed to `open_path`/`open_path_ensure` are now normalized to the connection's established socket family too, the same way `local_ip` already was. This is scoped narrowly, exactly as #787 worked out: `Connection::new` (path 0, which establishes the family in the first place), `handle_event`'s incoming datagram arm, and `handle_first_packet` are all left untouched for `remote` -- their remote addresses come from the OS's own recvfrom-equivalent, which for a single bound socket already reports peer addresses in one consistent representation. With every FourTuple that ever enters Connection-owned state now consistently normalized -- both remote (this commit) and local_ip (already normalized at all five entry points) -- structural equality just works everywhere. Delete FourTuple::is_same_remote()/ canonical_remote() entirely and revert every comparison site (early_discard_packet, PATH_CHALLENGE-on-active-path detection, OBSERVED_ADDR matching, the peer-migration trigger, PathResponses::push, is_probably_same_path) to plain ==/!=. There is no comparison-time canonicalization hack left anywhere in noq-proto. Added regression coverage exercised directly through Connection::open_path/open_path_ensure (bypassing the noq wrapper): normalization for both a dual-stack and an IPv4-only connection, and an asymmetric-routing test (ManyToManyRouting with the outbound leg reachable via plain IPv4 but the return leg only via mapped IPv4-in-IPv6) proving this actually closes a real gap -- verified by temporarily reverting the remote normalization and confirming failure first (a path that times out and never validates), then confirming it passes restored. This is the last piece #787 explored separately; consolidating it here so #784 is the complete fix and #787 can close as superseded. --- noq-proto/src/connection/mod.rs | 80 +++++++++++++----- noq-proto/src/connection/paths.rs | 21 +++-- noq-proto/src/lib.rs | 109 +++++-------------------- noq-proto/src/tests/multipath.rs | 130 ++++++++++++++++++++++++++++++ noq/src/connection.rs | 19 ++--- 5 files changed, 228 insertions(+), 131 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 47704a861b..47c0b51a10 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3,7 +3,7 @@ use std::{ collections::{BTreeMap, VecDeque, btree_map}, convert::TryFrom, fmt, io, mem, - net::{IpAddr, SocketAddr}, + net::{IpAddr, SocketAddr, SocketAddrV6}, num::{NonZeroU32, NonZeroUsize}, sync::Arc, }; @@ -551,7 +551,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result<(PathId, bool), PathError> { - let network_path = self.normalize_network_path_local_ip(network_path); + let network_path = self.normalize_network_path(network_path)?; let existing_open_path = self.paths.iter().find(|(id, path)| { network_path.is_probably_same_path(&path.data.network_path) && !self.abandoned_paths.contains(id) @@ -573,7 +573,7 @@ impl Connection { initial_status: PathStatus, now: Instant, ) -> Result { - let network_path = self.normalize_network_path_local_ip(network_path); + let network_path = self.normalize_network_path(network_path)?; let Some(max_path_id) = self.max_path_id() else { return Err(PathError::MultipathNotNegotiated); }; @@ -2349,7 +2349,7 @@ impl Connection { // forbids migration, drop the datagram. This could be relaxed to heuristically // permit NAT-rebinding-like migration. if let Some(known_path) = self.path_mut(path_id) { - if !network_path.is_same_remote(&known_path.network_path) && !peer_may_probe { + if network_path.remote != known_path.network_path.remote && !peer_may_probe { trace!( %path_id, %network_path, @@ -5008,7 +5008,7 @@ impl Connection { let path = &mut self .path_mut(path_id) .expect("payload is processed only after the path becomes known"); - if network_path.is_same_remote(&path.network_path) { + if network_path.remote == path.network_path.remote { // PATH_CHALLENGE on active path, possible off-path packet // forwarding attack. Send a non-probing packet to recover the // active path. See @@ -5305,7 +5305,7 @@ impl Connection { let space_open_status = self.spaces[SpaceKind::Data].for_path(path_id).open_status; let path = self.path_data_mut(path_id); - if path.network_path.is_same_remote(&network_path) { + if path.network_path.remote == network_path.remote { if let Some(updated) = path.update_observed_addr_report(observed) && space_open_status == OpenStatus::Informed { @@ -5603,10 +5603,7 @@ impl Connection { if self.peer_may_migrate() && (migrate_on_any_packet || !is_probing_packet) && is_largest_received_pn - && !self - .path_data(path_id) - .network_path - .is_same_remote(&network_path) + && self.path_data(path_id).network_path.remote != network_path.remote { self.migrate(path_id, now, network_path, migration_observed_addr); // Break linkability, if possible @@ -7099,25 +7096,66 @@ impl Connection { self.established_ipv6 = ipv6; } - /// Normalizes caller- or OS-supplied local IPs at `Connection` entry boundaries. + /// Normalizes caller-supplied path addresses at `open_path` entry boundaries. + /// + /// `open_path` and `open_path_ensure` receive caller-supplied `remote` addresses, which may + /// use either plain IPv4 or IPv4-mapped-IPv6 representation. Normalize both `remote` and + /// `local_ip` to the connection's established socket family before storing or comparing them + /// (#738, #784). + fn normalize_network_path(&self, network_path: FourTuple) -> Result { + let ipv6 = self.is_ipv6(); + let remote = Self::normalize_remote_to_socket_family(network_path.remote, ipv6)?; + let local_ip = network_path + .local_ip + .map(|ip| Self::normalize_local_ip(ip, ipv6)); + Ok(FourTuple::new(remote, local_ip)) + } + + fn normalize_remote_to_socket_family( + remote: SocketAddr, + ipv6: bool, + ) -> Result { + if ipv6 { + Ok(SocketAddr::V6(Self::ensure_ipv6(remote))) + } else { + match remote { + SocketAddr::V4(_) => Ok(remote), + SocketAddr::V6(v6) => v6 + .ip() + .to_ipv4_mapped() + .map(|ip| SocketAddr::new(IpAddr::V4(ip), v6.port())) + .ok_or(PathError::InvalidRemoteAddress(remote)), + } + } + } + + fn ensure_ipv6(remote: SocketAddr) -> SocketAddrV6 { + match remote { + SocketAddr::V6(v6) => v6, + SocketAddr::V4(v4) => SocketAddrV6::new(v4.ip().to_ipv6_mapped(), v4.port(), 0, 0), + } + } + + /// Normalizes OS-supplied local IPs at `Connection` entry boundaries. /// /// `PathData.network_path.local_ip` is now normalized before it enters /// connection-owned state, so plain `.local_ip ==` comparisons are safe inside /// `noq-proto` (#738, #784). This normalization is intentionally based on the - /// connection's established socket family: on a dual-stack socket, a plain IPv4 - /// `local_ip` is mapped to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is - /// folded to plain IPv4. + /// connection's established socket family: on a dual-stack socket, a plain IPv4 `local_ip` is + /// mapped to IPv6; on an IPv4-only socket, a mapped-IPv4 `local_ip` is folded to plain IPv4. /// /// This does change what is handed to the OS. `PathData.network_path.local_ip` becomes /// `Transmit::src_ip`, and the UDP backend sends different control messages for /// `IpAddr::V4` and `IpAddr::V6` (`IP_PKTINFO` vs `IPV6_PKTINFO` on Unix). That is the - /// intended behavior for a dual-stack socket: sending IPv4 packet info while the - /// destination sockaddr is IPv6-family is a plausible reason the OS would ignore or - /// mishandle source selection for #738, though this root-cause hypothesis has not - /// been verified on real multi-interface hardware. `remote` is deliberately not - /// normalized here because `PathData.network_path.remote` also drives - /// `Transmit::destination`; see #787 for an alternative that also normalizes `remote` - /// at the connection's established socket family. + /// intended behavior for a dual-stack socket: sending IPv4 packet info while the destination + /// sockaddr is IPv6-family is a plausible reason the OS would ignore or mishandle source + /// selection for #738, though this root-cause hypothesis has not been verified on real + /// multi-interface hardware. + /// + /// `remote` is normalized by [`Self::normalize_network_path`] for caller-supplied + /// `open_path`/`open_path_ensure` inputs. It is intentionally preserved here because incoming + /// datagrams come from the OS receive metadata, which is already consistent for a single bound + /// socket, and because `PathData.network_path.remote` drives `Transmit::destination`. fn normalize_network_path_local_ip(&self, network_path: FourTuple) -> FourTuple { Self::normalize_network_path_local_ip_for_family(network_path, self.is_ipv6()) } diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index b349e60f58..0375a504d2 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -875,7 +875,7 @@ impl PathResponses { let existing = self .pending .iter_mut() - .find(|x| x.network_path.is_same_remote(&network_path)); + .find(|x| x.network_path.remote == network_path.remote); if let Some(existing) = existing { // Update a queued response if existing.packet <= packet { @@ -896,9 +896,8 @@ impl PathResponses { let response = *self.pending.last()?; // We use an exact comparison here. By the time connection-owned path state and // received PATH_CHALLENGE metadata reach this queue, `Connection` has normalized - // `local_ip` to the established socket family. `push` still coalesces - // mapped-vs-plain `remote` representations, but exact matching here remains the - // right on-path/off-path split because different `local_ip`s can be different + // `remote` and `local_ip` to the established socket family. Exact matching here remains + // the right on-path/off-path split because different `local_ip`s can be different // interfaces. if response.network_path == network_path { // We don't bother searching further because we expect that the on-path response will @@ -1205,16 +1204,16 @@ mod tests { assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX) } - /// Regression test (#738): `PATH_CHALLENGE`s on mapped-vs-plain-IPv4 FourTuples of - /// the same peer must coalesce into one pending response, not two. + /// `PATH_CHALLENGE`s from the same remote must coalesce into one pending response. #[test] - fn push_coalesces_mapped_v4_representation() { - let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); - let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); + fn push_coalesces_same_remote() { + let remote = "1.2.3.4:443".parse().unwrap(); + let path = FourTuple::from_remote(remote); + let alternate_local = FourTuple::new(remote, Some("192.0.2.1".parse().unwrap())); let mut responses = PathResponses::default(); - responses.push(1, 0xaaaa, mapped); - responses.push(2, 0xbbbb, plain); + responses.push(1, 0xaaaa, path); + responses.push(2, 0xbbbb, alternate_local); assert_eq!(responses.pending.len(), 1); assert_eq!(responses.pending[0].token, 0xbbbb); diff --git a/noq-proto/src/lib.rs b/noq-proto/src/lib.rs index 36a18f2089..8b9b399596 100644 --- a/noq-proto/src/lib.rs +++ b/noq-proto/src/lib.rs @@ -368,15 +368,10 @@ const MAX_STREAM_COUNT: u64 = 1 << 60; /// /// `FourTuple` implements `From`, which expands to [`Self::from_remote`]. /// -/// `noq-proto::Connection` normalizes `local_ip` at every entry boundary before storing -/// a `FourTuple` in connection-owned state, so plain `.local_ip ==` comparisons are safe -/// there (#738 / #784). `remote` is not yet normalized at the `noq-proto` level because -/// `PathData.network_path.remote` also drives `Transmit::destination` for actual OS -/// sends; see #787 for an alternative that also normalizes `remote` at the connection's -/// established socket family. Until this scope is extended (or superseded), -/// connection-internal remote comparisons that need to ignore mapped-vs-plain IPv4 -/// representation differences use `Self::is_same_remote` (private, not linked here -/// since it would produce a broken intra-doc link in the published docs). +/// `noq-proto::Connection` normalizes caller-supplied `remote` addresses at +/// `open_path`/`open_path_ensure` and `local_ip` at every entry boundary before storing a +/// `FourTuple` in connection-owned state, so plain structural comparisons are safe there (#738, +/// #784). #[derive(Hash, Eq, PartialEq, Copy, Clone)] pub struct FourTuple { /// The remote side of this tuple. @@ -432,35 +427,6 @@ impl FourTuple { self.local_ip } - /// Canonicalizes `remote` for comparison/hashing purposes (see the type-level docs - /// above, #738). `to_canonical()` folds `::ffff:a.b.c.d` down to plain - /// IPv4; the scope_id is kept for addresses that stay IPv6, because [`Self::new`] - /// deliberately preserves it for link-local and multicast remotes — two different - /// link-local interfaces must not compare equal just because their canonicalized - /// IP matches. A mapped address canonicalizes to V4, which has no scope, so it - /// gets 0. - fn canonical_remote(&self) -> (IpAddr, u16, u32) { - let ip = self.remote.ip().to_canonical(); - let scope = match self.remote { - SocketAddr::V6(v6) if ip.is_ipv6() => v6.scope_id(), - _ => 0, - }; - (ip, self.remote.port(), scope) - } - - /// Whether `self` and `other` share the same remote peer, ignoring the - /// mapped-IPv4-vs-plain-IPv4 representation difference that motivated this (#738). - /// - /// `local_ip` is now normalized at every `Connection` entry boundary (see - /// `Connection::normalize_network_path_local_ip`), so plain `.local_ip ==` - /// comparisons are safe there. `remote` is NOT yet normalized at the `noq-proto` - /// level because `PathData.network_path.remote` also drives `Transmit::destination` - /// for actual OS sends; see #784, and #787 for an alternative that also normalizes - /// `remote` at the connection's established socket family. - pub(crate) fn is_same_remote(&self, other: &Self) -> bool { - self.canonical_remote() == other.canonical_remote() - } - /// Returns whether we think the other address probably represents the same path /// as ours. /// @@ -472,7 +438,7 @@ impl FourTuple { /// - `a.is_probably_same_path(b)` /// - `b.is_probably_same_path(a)` pub(crate) fn is_probably_same_path(&self, other: &Self) -> bool { - self.is_same_remote(other) && (self.local_ip.is_none() || self.local_ip == other.local_ip) + self.remote == other.remote && (self.local_ip.is_none() || self.local_ip == other.local_ip) } } @@ -508,72 +474,36 @@ impl From for FourTuple { mod four_tuple_tests { use super::*; - /// Mapped vs. plain IPv4 representations of the same peer must compare equal via - /// [`FourTuple::is_same_remote`] (structural `PartialEq` stays derived and unaware - /// of this -- see the type-level docs above) (#738). - #[test] - fn is_same_remote_ignores_mapped_v4_representation() { - let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); - let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); - assert!(mapped.is_same_remote(&plain)); - assert!(plain.is_same_remote(&mapped)); - assert_ne!(mapped, plain, "structural PartialEq must stay derived"); - } - - /// Regression test (#738): link-local `FourTuple`s differing only in `scope_id` - /// (different interfaces) must NOT compare same via `is_same_remote` -- an - /// earlier fix draft dropped `scope_id` entirely, collapsing distinct - /// interfaces into one path. + /// Link-local `FourTuple`s differing only in `scope_id` describe different + /// interfaces and must stay distinct. #[test] - fn is_same_remote_preserves_link_local_scope_id() { + fn new_preserves_link_local_scope_id() { let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); let iface_b = FourTuple::from_remote("[fe80::1%5]:443".parse().unwrap()); - assert!(!iface_a.is_same_remote(&iface_b)); - assert!(!iface_b.is_same_remote(&iface_a)); + assert_ne!(iface_a, iface_b); } - /// Inverse of [`is_same_remote_preserves_link_local_scope_id`]: for a *global* - /// IPv6 address, `FourTuple::new()` already zeroes `scope_id` on construction, - /// so bogus input scope_ids must still compare equal here (both via - /// `is_same_remote` and structural `PartialEq`, since `new()` normalizes the - /// stored value in this case). + /// For a global IPv6 address, `FourTuple::new()` zeroes `scope_id` on construction. #[test] - fn is_same_remote_zeroes_scope_id_for_global_v6() { + fn new_zeroes_scope_id_for_global_v6() { let a = FourTuple::from_remote("[2001:db8::1%3]:443".parse().unwrap()); let b = FourTuple::from_remote("[2001:db8::1%5]:443".parse().unwrap()); - assert!(a.is_same_remote(&b)); assert_eq!(a, b); } - /// Mirrors [`is_same_remote_preserves_link_local_scope_id`] for multicast - /// addresses. + /// Multicast `FourTuple`s differing only in `scope_id` describe different interfaces and must + /// stay distinct. #[test] - fn is_same_remote_preserves_multicast_scope_id() { + fn new_preserves_multicast_scope_id() { let iface_a = FourTuple::from_remote("[ff02::1%3]:443".parse().unwrap()); let iface_b = FourTuple::from_remote("[ff02::1%5]:443".parse().unwrap()); - assert!(!iface_a.is_same_remote(&iface_b)); - assert!(!iface_b.is_same_remote(&iface_a)); - } - - /// `is_probably_same_path` re-implements the canonicalizing `remote` - /// comparison independently of `PartialEq`, so it needs its own coverage. - /// `local_ip: None` here; see - /// [`is_probably_same_path_ignores_mapped_v4_representation_for_local_ip`] for - /// the `Some` case. - #[test] - fn is_probably_same_path_ignores_mapped_v4_representation() { - let mapped = FourTuple::from_remote("[::ffff:1.2.3.4]:443".parse().unwrap()); - let plain = FourTuple::from_remote("1.2.3.4:443".parse().unwrap()); - assert!(mapped.is_probably_same_path(&plain)); - assert!(plain.is_probably_same_path(&mapped)); + assert_ne!(iface_a, iface_b); } - /// The `local_ip: Some(..)` counterpart of - /// [`is_probably_same_path_ignores_mapped_v4_representation`]: representation - /// differences in `local_ip` are deliberately not hidden here. `Connection` - /// normalizes local IPs at its entry boundaries before storing them. + /// Representation differences are not hidden here. `Connection` normalizes path addresses at + /// its entry boundaries before storing them. #[test] - fn is_probably_same_path_uses_structural_local_ip_equality() { + fn is_probably_same_path_uses_structural_equality() { let remote = "9.9.9.9:443".parse().unwrap(); let mapped = FourTuple::new(remote, Some("::ffff:1.2.3.4".parse().unwrap())); let plain = FourTuple::new(remote, Some("1.2.3.4".parse().unwrap())); @@ -581,8 +511,7 @@ mod four_tuple_tests { assert!(!plain.is_probably_same_path(&mapped)); } - /// Same as above for `scope_id`. Only `remote` can vary here -- `local_ip: - /// IpAddr` has no `scope_id` field. + /// `is_probably_same_path` still distinguishes scoped link-local remotes. #[test] fn is_probably_same_path_distinguishes_link_local_scope_id() { let iface_a = FourTuple::from_remote("[fe80::1%3]:443".parse().unwrap()); diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index ae7da23be2..8415a146e8 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -417,6 +417,92 @@ fn open_path_normalizes_local_ip_for_dual_stack_connection() -> TestResult { Ok(()) } +#[test] +fn open_path_normalizes_remote_for_dual_stack_connection() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + assert!(pair.conn(Client).is_ipv6()); + + let remote_ip = Ipv4Addr::new(192, 0, 2, 2); + let plain_remote = SocketAddr::new(remote_ip.into(), 4433); + let mapped_remote = SocketAddr::new(remote_ip.to_ipv6_mapped().into(), 4433); + let path_id = pair.open_path( + Client, + FourTuple::from_remote(plain_remote), + PathStatus::Available, + )?; + + assert_eq!(pair.network_path(Client, path_id)?.remote(), mapped_remote); + + let (same_path_id, existed) = pair.open_path_ensure( + Client, + FourTuple::from_remote(mapped_remote), + PathStatus::Available, + )?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.remote(), + mapped_remote + ); + + Ok(()) +} + +#[test] +fn open_path_normalizes_remote_for_ipv4_connection() -> TestResult { + let _guard = subscribe(); + let transport = Arc::new(TransportConfig { + max_concurrent_multipath_paths: NonZeroU32::new(MAX_PATHS), + ..TransportConfig::default() + }); + let server_cfg = ServerConfig { + transport: transport.clone(), + ..server_config() + }; + let client_cfg = ClientConfig { + transport, + ..client_config() + }; + let server = Endpoint::new(Default::default(), Some(Arc::new(server_cfg)), false); + let client = Endpoint::new(Default::default(), None, false); + let mut pair = Pair::new_from_endpoint(client, server); + pair.routes = + ManyToManyRouting::from_routes([("1.1.1.1:44433", 0)], [("2.2.2.1:4433", 0)]).into(); + let (client_ch, server_ch) = pair.connect_with(client_cfg); + let mut pair = ConnPair::new(pair, client_ch, server_ch); + + assert!(!pair.conn(Client).is_ipv6()); + + let remote_ip = Ipv4Addr::new(192, 0, 2, 2); + let plain_remote = SocketAddr::new(remote_ip.into(), 4433); + let mapped_remote = SocketAddr::new(remote_ip.to_ipv6_mapped().into(), 4433); + let path_id = pair.open_path( + Client, + FourTuple::from_remote(mapped_remote), + PathStatus::Available, + )?; + + assert_eq!(pair.network_path(Client, path_id)?.remote(), plain_remote); + + let (same_path_id, existed) = pair.open_path_ensure( + Client, + FourTuple::from_remote(plain_remote), + PathStatus::Available, + )?; + + assert!(existed); + assert_eq!(same_path_id, path_id); + assert_eq!( + pair.network_path(Client, same_path_id)?.remote(), + plain_remote + ); + + Ok(()) +} + #[test] fn connection_socket_family_is_fixed_at_establishment() -> TestResult { let _guard = subscribe(); @@ -441,6 +527,50 @@ fn connection_socket_family_is_fixed_at_establishment() -> TestResult { Ok(()) } +#[test] +fn open_path_with_plain_remote_validates_when_response_uses_mapped_ipv4() -> TestResult { + let _guard = subscribe(); + let client_addr_0 = "[::ffff:1.1.1.0]:44433".parse::()?; + let server_addr_0 = "[::ffff:2.2.2.0]:4433".parse::()?; + let client_addr_1 = "[::ffff:1.1.1.1]:44433".parse::()?; + let server_addr_1_mapped = "[::ffff:2.2.2.1]:4433".parse::()?; + let server_addr_1_plain = "2.2.2.1:4433".parse::()?; + let mut pair = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .with_routes(ManyToManyRouting::from_routes( + [(client_addr_0, 0), (client_addr_1, 2)], + [ + (server_addr_0, 0), + (server_addr_1_plain, 1), + (server_addr_1_mapped, 1), + ], + )) + .connect(); + + let path_id = pair.open_path( + Client, + FourTuple::from_remote(server_addr_1_plain), + PathStatus::Available, + )?; + + assert!( + !pair.drive_bounded(100), + "path validation should become idle" + ); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Established { id })) if id == path_id + ); + + Ok(()) +} + #[test] fn open_path_with_plain_local_ip_validates_on_mapped_ipv4_route() -> TestResult { let _guard = subscribe(); diff --git a/noq/src/connection.rs b/noq/src/connection.rs index a4432ee8e0..9c6765dedf 100644 --- a/noq/src/connection.rs +++ b/noq/src/connection.rs @@ -960,15 +960,16 @@ impl Connection { /// was canonicalised (#738). This is the one place where addresses supplied by the /// caller (as opposed to observed from the OS) enter [`Connection`], so keeping /// `local_ip` in the same representation as `remote` here avoids relying on every -/// downstream comparison to canonicalize it individually. Note: unlike the -/// `noq-proto`-level comparison fixes for #738 (which were verified against the -/// actual reported failure on real Android hardware with physical Wi-Fi/cellular -/// interfaces), this specific normalization was not independently verified to fix -/// #738's reported symptom on its own -- see the doc comment on the `noq` crate's -/// `open_path_with_explicit_ipv4_local_ip_on_dualstack_socket` test for what was and -/// wasn't reproducible in a loopback-only environment. It is included because it -/// closes a real inconsistency with `remote`'s handling in this same function, -/// independent of whether it's also part of #738's root cause. +/// downstream comparison to canonicalize it individually. The proto layer now also normalizes +/// caller-supplied `remote` addresses at `open_path`/`open_path_ensure`, closing the same +/// representation gap there (#784). Note: unlike the proto-level storage normalization for #738 +/// (which was verified against the actual reported failure on real Android hardware with physical +/// Wi-Fi/cellular interfaces), this wrapper-level `local_ip` normalization was not independently +/// verified to fix #738's reported symptom on its own -- see the doc comment on the `noq` crate's +/// `open_path_with_explicit_ipv4_local_ip_on_dualstack_socket` test for what was and wasn't +/// reproducible in a loopback-only environment. It is included because it closes a real +/// inconsistency with `remote`'s handling in this same function, independent of whether it's also +/// part of #738's root cause. fn normalize_network_path( network_path: FourTuple, conn: &proto::Connection,