Fix REGISTER's Via header ignoring the target's actual transport - #141
Open
youngnsx wants to merge 1 commit into
Open
Fix REGISTER's Via header ignoring the target's actual transport#141youngnsx wants to merge 1 commit into
youngnsx wants to merge 1 commit into
Conversation
Registration::register() built its Via header via `self.endpoint.get_via(None, None)` before the request's actual transport had been resolved. get_via's `None` fallback always uses the endpoint's first-bound listener — in any client that binds UDP upfront (the common case for a register/dial-only role, no inbound listener needed) that's the UDP transport, regardless of what `;transport=` the target URI actually asked for. Symptom: registering against a `sip:host;transport=tcp` target correctly dials a real TCP connection and sends the REGISTER over it, but the message's own Via header claims `SIP/2.0/UDP` with the client's unrelated UDP transport's address. A spec-compliant server receiving a request whose declared Via transport doesn't match the connection it actually arrived on is free to treat that as malformed and drop it silently — confirmed against a real deployment (FreeSWITCH/sofia-sip): TCP handshake completes, the REGISTER is received intact and logged, but the server never responds — no error, nothing in its own logs beyond the raw bytes arriving. From the client's side that's indistinguishable from "the server doesn't support TCP", which is what it looks like until you compare the exact bytes received against what a spec-correct request looks like. Fix: resolve (and, for TCP/TLS/WS/WSS, lazily dial+cache — the same lookup Transaction::send() would perform anyway, so this doesn't add a second real connection attempt on the happy path) the target's connection first, and build Via from that connection's real local SipAddr instead. A target with no `;transport=` param still falls through to the existing bound UDP listener exactly as before (see TransportLayerInner::lookup's `first_udp` fallback), so this is safe to apply unconditionally rather than gating it on transport type. Falls back to the old `get_via(None, None)` behavior if the early lookup itself fails, rather than surfacing the error twice — the identical lookup happens again inside Transaction::send() regardless, so a real failure (unreachable target, DNS failure, etc.) still surfaces normally through the existing error path. cargo test --lib: 269/269 passing, no regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Registration::register()builds its Via header viaself.endpoint.get_via(None, None)before the request's actualtransport has been resolved.
get_via'sNonefallback always usesthe endpoint's first-bound listener — in any client that binds UDP
upfront (the common case for a register/dial-only role, no inbound
listener needed) that's the UDP transport, regardless of what
;transport=the target URI actually asked for.Symptom
Registering against a
sip:host;transport=tcptarget correctlydials a real TCP connection and sends the REGISTER over it, but the
message's own Via header claims
SIP/2.0/UDPwith the client'sunrelated UDP transport's address.
A spec-compliant server receiving a request whose declared Via
transport doesn't match the connection it actually arrived on is
free to treat that as malformed and drop it silently. Confirmed
against a real deployment (FreeSWITCH/sofia-sip): TCP handshake
completes, the REGISTER is received intact and logged, but the
server never responds — no error, nothing distinguishable in the
server's own logs from the request never having arrived. From the
client's side that's indistinguishable from "the server doesn't
support TCP", until you compare the exact bytes received against
what a spec-correct request looks like.
Fix
Resolve (and, for TCP/TLS/WS/WSS, lazily dial+cache — the same
lookup
Transaction::send()would perform anyway, so this doesn'tadd a second real connection attempt on the happy path) the target's
connection first, and build Via from that connection's real local
SipAddrinstead. A target with no;transport=param still fallsthrough to the existing bound UDP listener exactly as before (see
TransportLayerInner::lookup'sfirst_udpfallback), so this issafe to apply unconditionally rather than gating it on transport
type. Falls back to the old
get_via(None, None)behavior if theearly lookup itself fails, rather than surfacing the error twice —
the identical lookup happens again inside
Transaction::send()regardless, so a real failure (unreachable target, DNS failure,
etc.) still surfaces normally through the existing error path.
Testing
cargo test --lib: 269/269 passing, no regressions.registration hung for the full 32s Timer B and returned a local
408 with no server response ever received. After the fix, the
same registration completes a full 401 challenge → digest retry →
server response round trip in ~0.16s, with the server's own trace
log confirming
Via: SIP/2.0/TCP ...now matches the connectionit actually arrived on.
Found while debugging a downstream app (a Tauri SIP softphone) whose
TCP/TLS registration path had been shipped as "implemented and
verified" based on a misread of this exact symptom as a server-side
limitation.