[asterisk-dev] [Code Review]: chan_sip REFER deadlock fixes

[David Vossel]

> On Aug. 2, 2011, 1:55 p.m., mjordan wrote:
> > /branches/1.8/channels/chan_sip.c, line 22928
> > <https://reviewboard.asterisk.org/r/1339/diff/1/?file=17695#file17695line22928>
> >
> >     Can the caller ID change between where you previously extracted it and the localtransfer?  You duplicate it previously, then lock p and reextract it here.
> 
> David Vossel wrote:
>     callid in this case is not Caller ID.  This field represents the Call-ID header in SIP messages that is part of uniquely identifying message belonging to a sip dialog.  It will stay consistent for the duration of the sip pvt's life.
> 
> mjordan wrote:
>     Got it.  I'm assuming then that we expect p->callid to be updated between where we last duplicated it out and at this point in the code.  While I know you've declared char *callid to be local to this if ... scope, it might be clearer if the variable had a different name - at first glance, it looks like you're duplicating p->callid back into the same variable.

Ha, I see exactly what you are talking about now.  Yes, that is dumb. I'll fix it.  I probably originally thought I just had to do it within that if statement, then realized it had to be in a larger scope and forgot to remove it.

You can highlight multiple lines by the way.  Just click and drag.  That helps give me more of the context surrounding the code you are commenting about.


- David


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


On Aug. 2, 2011, 12:21 p.m., David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1339/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2011, 12:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> handle_request_refer() is a complete mess when it comes to locking.  A deadlock occurs, we fix it, and then it moves somewhere else.  This patch attempts to resolve all the possible locking inversion issues that can occur in this function.
> 
> 
> This addresses bug ASTERISK-18082.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18082
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 330578 
> 
> Diff: https://reviewboard.asterisk.org/r/1339/diff
> 
> 
> Testing
> -------
> 
> I tested refer using a snom phone with blind transfer, but that is not very impressive.
> 
> James Van Vleet has tested this code using a load testing tool that was capable of exposing all sorts of problems.  He has reported that his test is running without issue using this iteration of the patch.  Given what it was capable of exposing earlier, I am confident in these test results.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110802/93848afc/attachment.htm>