fix(sip): answer re-INVITE with the RFC 3264 direction, not the cached SDP - #780
fix(sip): answer re-INVITE with the RFC 3264 direction, not the cached SDP#780prasanth-33460 wants to merge 1 commit into
Conversation
…d SDP
A hold re-INVITE offers a=sendonly. Both re-INVITE paths answered by replaying
the cached local SDP verbatim, which still carries a=sendrecv. RFC 3264 §6.1
requires a sendonly offer to be answered recvonly, so the answer is invalid and
the carrier tears the dialog down ~60ms later. On a bridged call (two SIP legs in
one room) that BYE collapses the whole call, so pressing HOLD hangs up.
Reproduced on v1.8.0 and v1.9.0 against two carrier SBCs. Captured:
IN INVITE a=sendonly <- carrier: hold
OUT 200 OK a=sendrecv <- invalid answer
IN ACK
IN BYE <- 63ms later
Consistent with the cause, there were no occurrences of sendonly/recvonly/
sendrecv/inactive in non-test source: direction handling was not implemented for
re-INVITEs.
answerDirectionFor maps the offer's direction to the correct answer
(sendonly->recvonly, recvonly->sendonly, inactive->inactive) and returns "" when
the offer is sendrecv or states no direction. withSDPDirection then rewrites the
single a= line in the cached SDP, appending one if absent and preserving CRLF.
Two deliberate choices:
* A text-level rewrite rather than parse/re-serialize. The cached SDP is an
already-negotiated, working body; reserializing risks perturbing codec/ptime/
connection lines the far end has accepted, so only the direction changes.
* A no-op when the offer states no direction (sendrecv is implied by RFC 4566
§6), so every non-hold re-INVITE — codec renegotiation, port change, session
timer refresh — behaves exactly as before.
Tests cover the captured carrier SDP, the full direction mapping, the
no-direction passthrough, unhold, appending a missing direction line, and
collapsing duplicates.
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| func answerDirectionFor(offer []byte) string { | ||
| // Scan media-level then session-level attributes; last one wins, matching how the | ||
| // direction applies to the (single) audio stream we negotiate. | ||
| dir := "" | ||
| for _, line := range strings.Split(string(offer), "\n") { | ||
| switch strings.TrimSpace(line) { | ||
| case "a=sendonly": | ||
| dir = "sendonly" | ||
| case "a=recvonly": | ||
| dir = "recvonly" | ||
| case "a=inactive": | ||
| dir = "inactive" | ||
| case "a=sendrecv": | ||
| dir = "sendrecv" | ||
| } | ||
| } | ||
| switch dir { | ||
| case "sendonly": | ||
| return "recvonly" // remote holds us: it sends, we only receive | ||
| case "recvonly": | ||
| return "sendonly" | ||
| case "inactive": | ||
| return "inactive" | ||
| default: | ||
| return "" // absent or sendrecv -> no rewrite needed | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Hold answer can pick the wrong media direction when a call offer contains more than one media stream
The direction to answer with is chosen by scanning the whole incoming offer and keeping the last direction found (answerDirectionFor at pkg/sip/inbound.go:378-404) instead of the one belonging to the audio stream, so an offer that carries a second stream with a different direction makes the audio reply say the wrong thing.
Impact: On calls where the far end includes an extra (e.g. video or image/T.38) stream in its hold or re-negotiation request, audio can be answered as inactive/wrong direction and the caller hears silence or the far end drops the call.
Why last-line-wins parsing breaks with multiple m= sections
answerDirectionFor splits the entire SDP body into lines and records every a=sendonly|recvonly|inactive|sendrecv it sees, letting the last one win (pkg/sip/inbound.go:381-393). SDP direction attributes are scoped: a session-level attribute is the default, and each m= section may override it. With an offer such as:
m=audio 29076 RTP/AVP 0
a=sendonly
m=video 0 RTP/AVP 96
a=inactive
the function returns inactive, and withSDPDirection (pkg/sip/inbound.go:409-442) then rewrites our single audio section to a=inactive, halting media in both directions rather than answering recvonly.
withSDPDirection has the mirror-image limitation: it replaces the first direction line found anywhere (possibly a session-level one) and silently drops all subsequent direction lines, which would corrupt a multi-section local body.
A fix is to track the current section while scanning (session-level default, then the first m=audio section's own attribute overriding it) and to apply the rewrite only inside that audio section.
Prompt for agents
answerDirectionFor in pkg/sip/inbound.go flattens the whole SDP offer and takes the last direction attribute it encounters, ignoring SDP scoping rules (session-level attribute is a default; each m= section can override it, and attributes belong to the section they follow). If a carrier's re-INVITE contains more than one media section (e.g. audio sendonly plus a disabled video or T.38 image section with its own direction), the direction of the last section wins and we answer the audio stream with the wrong direction. withSDPDirection has the mirror problem: it replaces the first direction line anywhere in the local body (possibly the session-level one) and drops every later direction line. Consider tracking section boundaries while scanning: pick the direction of the first m=audio section, falling back to the session-level attribute when the audio section has none, and confine the rewrite to that same audio section.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
The Failing test: Reproduced on clean
|
2 participants
Fixes #779.
Problem
A hold re-INVITE offers
a=sendonly. Both re-INVITE paths answer by replaying the cached local SDP verbatim, which still carriesa=sendrecv. RFC 3264 §6.1 requires asendonlyoffer to be answeredrecvonly, so the answer is invalid and the carrier tears the dialog down ~60 ms later.On a bridged call (two SIP legs in one room) that BYE collapses the whole call — so pressing hold hangs up.
Captured on v1.8.0 and v1.9.0, against two carrier SBCs:
The cached SDP is the previously negotiated
sendrecvanswer, so the direction attribute was never adjusted to the new offer:pkg/sip/inbound.go:422— inbound:cc.AcceptAsKeepAlive(existing.cc.OwnSDP())pkg/sip/inbound.go:434— outbound:cc.AcceptAsKeepAlive(localSDP)Consistent with that, there were no occurrences of
sendonly/recvonly/sendrecv/inactivein non-test source — direction handling wasn't implemented for re-INVITEs.Fix
Two helpers, used in both paths:
answerDirectionFor(offer)— maps the offer's direction to the correct answer (sendonly→recvonly,recvonly→sendonly,inactive→inactive), returning""when the offer issendrecvor states no direction.withSDPDirection(local, dir)— rewrites the singlea=direction line in the cached SDP, appending one if absent, preserving CRLF.Two deliberate choices, both open to discussion
1. Text-level rewrite rather than parse/re-serialize. The cached SDP is an already-negotiated, working body; reserializing risks perturbing codec /
ptime/ connection lines the far end has accepted, so only the direction attribute changes.If you'd rather this lived in the SDP layer (
media-sdk) where parsing already happens, that's very likely the better long-term home — happy to redo it there.2. No-op when the offer states no direction (
sendrecvis implied, RFC 4566 §6), so every non-hold re-INVITE — codec renegotiation, port change (#728), session-timer refresh — behaves exactly as before. That's the common case and I didn't want to change it.Tests
pkg/sip/sdp_hold_direction_test.gocovers the captured carrier SDP, the full direction mapping, the no-direction passthrough, unhold, appending a missing direction line, and collapsing duplicates. The existingpkg/sipsuite passes unchanged.Not verified
The SDP answer is now correct and hold no longer ends the call (confirmed on a live trunk). I have not characterised media behaviour across repeated hold/unhold cycles — whether RTP flow is torn down and re-established as the implementation might expect. Worth a maintainer's eye.