fix(proto): normalize FourTuple address family at Connection boundaries

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, SocketAddrV6},
     num::{NonZeroU32, NonZeroUsize},
     sync::Arc,
 };
@@ -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<PathId, PathState>,
+    /// 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,6 +333,9 @@ impl Connection {
         side_args: SideArgs,
         qlog: QlogSink,
     ) -> Self {
+        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);
@@ -366,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 {
@@ -430,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);
         }
@@ -537,6 +551,7 @@ impl Connection {
         initial_status: PathStatus,
         now: Instant,
     ) -> Result<(PathId, bool), PathError> {
+        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)
@@ -558,6 +573,7 @@ impl Connection {
         initial_status: PathStatus,
         now: Instant,
     ) -> Result<PathId, PathError> {
+        let network_path = self.normalize_network_path(network_path)?;
         let Some(max_path_id) = self.max_path_id() else {
             return Err(PathError::MultipathNotNegotiated);
         };
@@ -2220,6 +2236,12 @@ 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 +2361,7 @@ 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
+                && network_path.local_ip != known_path.network_path.local_ip
                 && !local_ip_may_migrate
             {
                 trace!(
@@ -3912,6 +3934,7 @@ impl Connection {
         packet: InitialPacket,
         remaining: Option<BytesMut>,
     ) -> 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());
@@ -5563,10 +5586,8 @@ impl Connection {
             && let Some(new_local_ip) = network_path.local_ip
         {
             let path_data = self.path_data_mut(path_id);
-            if path_data
-                .network_path
-                .local_ip
-                .is_some_and(|ip| ip != new_local_ip)
+            if path_data.network_path.local_ip.is_some()
+                && path_data.network_path.local_ip != network_path.local_ip
             {
                 debug!(
                     %path_id,
@@ -5582,7 +5603,7 @@ impl Connection {
         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
+            && 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
@@ -7063,14 +7084,101 @@ 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-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<FourTuple, PathError> {
+        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<SocketAddr, PathError> {
+        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.
+    ///
+    /// 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 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())
+    }
+
+    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.

noq-proto/src/connection/paths.rs

@@ -894,10 +894,11 @@ 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
+        // `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
             // get drained in the immediate future by a call to `pop_on_path`
@@ -1202,4 +1203,19 @@ mod tests {
         // outside range saturates
         assert_eq!(PathId::MAX.saturating_add(1u8), PathId::MAX)
     }
+
+    /// `PATH_CHALLENGE`s from the same remote must coalesce into one pending response.
+    #[test]
+    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, path);
+        responses.push(2, 0xbbbb, alternate_local);
+
+        assert_eq!(responses.pending.len(), 1);
+        assert_eq!(responses.pending[0].token, 0xbbbb);
+    }
 }

noq-proto/src/lib.rs

@@ -367,6 +367,11 @@ const MAX_STREAM_COUNT: u64 = 1 << 60;
 /// paths exist with the same remote, but different local IP interfaces.
 ///
 /// `FourTuple` implements `From<SocketAddr>`, which expands to [`Self::from_remote`].
+///
+/// `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.
@@ -464,3 +469,54 @@ impl From<SocketAddr> for FourTuple {
         Self::from_remote(value)
     }
 }
+
+#[cfg(test)]
+mod four_tuple_tests {
+    use super::*;
+
+    /// Link-local `FourTuple`s differing only in `scope_id` describe different
+    /// interfaces and must stay distinct.
+    #[test]
+    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_ne!(iface_a, iface_b);
+    }
+
+    /// For a global IPv6 address, `FourTuple::new()` zeroes `scope_id` on construction.
+    #[test]
+    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_eq!(a, b);
+    }
+
+    /// Multicast `FourTuple`s differing only in `scope_id` describe different interfaces and must
+    /// stay distinct.
+    #[test]
+    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_ne!(iface_a, iface_b);
+    }
+
+    /// 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_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));
+    }
+
+    /// `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());
+        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));
+    }
+}