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

[Terry Wilson]

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

Ship it!


>From the best I can tell after staring at the patch and RFC 3261 for a day, this seems to be correct. Or at least as correct as it can be given that chan_sip doesn't really have a transaction layer.

So, I'm going to give this a "Ship It" pending a few superfluos changes since I can't find anything seriously wrong with it. Great work!


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

    unsigned int



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

    unsigned int



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

    down the path



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

    unsigned int / %30u



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/776/#comment5416>

    


- Terry


On 2010-07-19 12:04:02, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/776/
> -----------------------------------------------------------
> 
> (Updated 2010-07-19 12:04:02)
> 
> 
> 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 277871 
>   /trunk/channels/sip/include/reqresp_parser.h 277871 
>   /trunk/channels/sip/include/sip.h 277871 
>   /trunk/channels/sip/reqresp_parser.c 277871 
> 
> 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
> 
>