Skip to content

fix(proto): canonicalize remote IP in early_discard_packet's peer check (noq#738) - #783

Open
cuzic wants to merge 5 commits into
n0-computer:mainfrom
cuzic:pr/noq-738-remote-canonicalize
Open

fix(proto): canonicalize remote IP in early_discard_packet's peer check (noq#738)#783
cuzic wants to merge 5 commits into
n0-computer:mainfrom
cuzic:pr/noq-738-remote-canonicalize

Conversation

@cuzic

@cuzic cuzic commented Aug 5, 2026

Copy link
Copy Markdown

Closes #738.

Problem

4bae6edd (the hotfix posted in #738) canonicalizes the local_ip
comparison in Connection::early_discard_packet, to handle dual-stack
sockets that report a peer as an IPv4-mapped-IPv6 address
(::ffff:a.b.c.d) on one side of the comparison and a plain IPv4 address
on the other. We applied it and confirmed on a real Android device
(WiFi + cellular) that it is not sufficient on its ownopen_path()
with an explicit local_ip still gets abandoned with ValidationFailed.

The remote comparison a few lines above the one 4bae6edd touches
has the exact same problem, and is not canonicalized:

if network_path.remote != known_path.network_path.remote && !peer_may_probe {

An incoming datagram's reported remote SocketAddr can compare unequal
to the known path's remote purely due to mapped-vs-plain representation,
so early_discard_packet silently drops every packet on that path before
it ever reaches PATH_RESPONSE / frame processing — same failure mode as
the local_ip bug, just one comparison earlier in the same function.

Fix

Mirror the existing local_ip fix: canonicalize both sides of the
remote comparison with IpAddr::to_canonical() before comparing.

This is the minimal, surgical fix — one comparison site, matching the
style of 4bae6edd. See #784 for an alternative that canonicalizes
once at FourTuple::new() construction time instead, which also covers
a few other latent comparison sites (Endpoint's
HashMap<FourTuple, ConnectionHandle> routing table,
is_probably_same_path, PATH_CHALLENGE on-path detection, OBSERVED_ADDRESS
matching, peer migration detection) that weren't yet observed failing in
our testing but share the same root cause.

Testing

  • Includes 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_routesfrom_routes now rejects
    the duplicate server_addr this test intentionally uses (invariant
    added by feat(proto): use path idle timeout for validation when opening a new path #721 after the regression test was originally written).
    Fails on unpatched main, passes with this fix.
  • cargo test -p noq-proto: 388 passed, 0 failed (full suite, not just
    the new test).
  • Verified on a real Android device: Secondary (local_ip: None)
    established, then PhysicalWifi/PhysicalCellular (local_ip
    explicit) both validate on the first attempt instead of retrying
    3x and getting abandoned:
    opening path "physical-wifi" -> <redacted>:45823 (local_ip=Some(192.168.10.80), attempt 1/3)
    path "physical-wifi" established: id=PathId(2)
    opening path "physical-cellular" -> <redacted>:45823 (local_ip=Some(10.209.235.90), attempt 1/3)
    path "physical-cellular" established: id=PathId(3)
    

divagant-martian and others added 5 commits July 30, 2026 22:31
…ck (noq#738)

The local_ip comparison a few lines below already canonicalizes both
sides via IpAddr::to_canonical() (4bae6ed) to handle dual-stack
sockets reporting IPv4-mapped-IPv6 addresses. The remote comparison
just above it does not, and can trip the same class of bug: an
incoming datagram's reported remote SocketAddr (e.g. [::ffff:a.b.c.d]:p)
compares unequal to the known path's remote (a.b.c.d:p) purely due to
representation, causing early_discard_packet to silently drop every
packet on that path before it ever reaches PATH_RESPONSE / frame
processing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

open_path() with explicit local_ip: PATH_RESPONSE never reaches on_path_response_received, path stuck ValidationFailed

3 participants