[asterisk-dev] [Code Review] SIP: transaction matching using top most Via header

[David Vossel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/776/#review2393
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/776/#comment5278>

    Great work David, but you forgot to destroy the iterator here.  Perhaps instead of having these terribly ugly goto statements you should create a separate function just for request to transaction/dialog matching.  That would make a lot more sense and make this much more readable/maintainable in the future.  Keep up the good work though!


- David


On 2010-07-14 17:31:20, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/776/
> -----------------------------------------------------------
> 
> (Updated 2010-07-14 17:31:20)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch modifies the way chan_sip.c does transaction to dialog matching.  Asterisk now stores information in the top most Via header of the initial incoming request and compares that against other Requests that have the same call-id.  This results in Asterisk being able to detect a forked call in which it has received multiple legs of the fork.  I completely stripped out the previous matching code and made the comparisons a little more explicit and easier to understand.  My comments in the code should offer all the details involving this patch.  
> 
> This patch also fixes a bug with the usage of the OBJ-MULTIPLE flag to find multiple dialogs with the same call-id.  Since the callback function was returning (CMP_MATCH | CMP_STOP) only the first item found was being returned.  I fixed this by making a new callback function for finding multiple dialogs that only returns (CMP_MATCH) on a match allowing for multiple items to be returned.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 276556 
>   /trunk/channels/sip/include/reqresp_parser.h 276556 
>   /trunk/channels/sip/include/sip.h 276556 
>   /trunk/channels/sip/reqresp_parser.c 276556 
> 
> Diff: https://reviewboard.asterisk.org/r/776/diff
> 
> 
> Testing
> -------
> 
> I tested this with various sip forking scenarios.  I created some of my own scenarios using sipp, and then set up an openSIPS proxy to fork all requests to my asterisk box and watch how they were handled.  Asterisk now only accepts the first request to a forked call and responds with a 482 (Loop Detected) to the second (or thrid or forth).  The 482 message is not entirely intuitive to me, but from what I understood that is what RFC 3261 section 8.2.2.2 indicates we should respond with in this scenario.
> 
> 8.2.2.2 Merged Requests
> 
>    If the request has no tag in the To header field, the UAS core MUST
>    check the request against ongoing transactions.  If the From tag,
>    Call-ID, and CSeq exactly match those associated with an ongoing
>    transaction, but the request does not match that transaction (based
>    on the matching rules in Section 17.2.3), the UAS core SHOULD
>    generate a 482 (Loop Detected) response and pass it to the server
>    transaction.
> 
>       The same request has arrived at the UAS more than once, following
>       different paths, most likely due to forking.  The UAS processes
>       the first such request received and responds with a 482 (Loop
>       Detected) to the rest of them.
> 
> 
> Thanks,
> 
> David
> 
>