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

[mjordan]

> 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.

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.


- mjordan


-----------------------------------------------------------
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/3b1cff64/attachment.htm>