[asterisk-dev] To-tag checks in chan_sip 1.8

[Olle E. Johansson]

On 18 Nov 2013, at 16:46, Mark Michelson <mmichelson at digium.com> wrote:

> On 11/18/2013 08:39 AM, Olle E. Johansson wrote:
>> Hi!
>> 
>> Someone (I can't track the change down in svn here) changed the to-tag handling in chan_sip to this:
>> 
>> /* Get their tag if we haven't already */
>>         if (ast_strlen_zero(p->theirtag) || (resp >= 200)) {
>>                 char tag[128];
>> 
>>                 gettag(req, "To", tag, sizeof(tag));
>>                 ast_string_field_set(p, theirtag, tag);
>>         } else {
>>                 /* Store theirtag to track for changes when 200 responses to invites are received without SDP */
>>                 ast_string_field_set(p, theirprovtag, p->theirtag);
>>         }
>> 
>> 
>> 
>> This change broke a lot of installations with forking proxys and PRACK. Before an established dialog, many to-tags can come in from various phones and servers ringing, but you lock when the dialog is established.
>> 
>> This code makes our responses use the first tag and not use the tag from the request we're responding to, which breaks communication. 
>> 
>> In order for me to fix this, I would need help in finding the commit and the reason for this change. Maybe we could change like this:
>> 
>> if (!ast_test_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED)) {
>> 
>> 
>> Thanks for your help!
>> 
>> /O
> Here's the review that resulted in that change:
> 
> https://reviewboard.asterisk.org/r/2827/
> 
> It was committed to 1.8 in revision 399989.
Thanks. Always interesting to work on networks with firewalls that do strange stuff...

> 
> You and I both shipped it. :)
(oh dear).

> 
> To sum up the change, the idea was that if we get a 200 OK with no SDP in response to on an outgoing initial INVITE, then we should still be able to accept the 200 OK if a provisional response was received that had an SDP. In order to make sure that the 200 OK was received from the same destination that sent the provisional response with SDP, we save the to-tag from the provisional response.
> 
> The problem, as you have noted, is that we can receive provisional responses from many destinations if the call is forked. I made the following note on the review:
> 
> " Our INVITE could be forked by a proxy to multiple UASs. If multiple destinations send 183 responses with SDP answers, then it becomes important not only that we negotiated an SDP but that we received an SDP on the particular branch that the 200 OK is received on. And as you know, chan_sip's transaction and forking awareness is pretty dismal."
> 
> The to-tag problem can be worked around (likely with the suggested patch in your e-mail). I just fear that in a forking scenario that there's more that can go wrong. To fix it correctly, we need proper transaction and forking support in chan_sip. Let's say we send an INVITE, it gets forked, and we receive 183 responses from destinations A, B, and C with SDPs a, b, and c respectively. Now destination B sends a 200 OK with no SDP. We now have to not only use the appropriate to-tag to match the incoming 200 OK, but we also have to apply SDP b to the outgoing call as well, even though the latest SDP we received may have been SDP a or c. This seems like a fairly reasonable scenario given the setup you have described.

Well, normally we don't answer a provisional response, but with PRACK we do, so the to-tag becomes more
important. I'll include this change in my PRACK branch. I am not sure that it will affect anything else in 1.8.
For later releases we might consider fixing this.

Again, thank you for your quick answer.

/O

> 
> Mark Michelson
> -- 
> _____________________________________________________________________
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
> 
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>   http://lists.digium.com/mailman/listinfo/asterisk-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131118/d7f85cc4/attachment-0001.html>