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

[mjordan]

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


More like questions than a code-review.


/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1339/#comment7805>

    It looks like (based on the comments) local_attended_transfer will lock p and p->owner.  If so, then is there a case where p->refer->attendedtransfer is false, and we attempt to unlock p and p->owner when they aren't locked?
    
    If this isn't the case, and the caller of this method locked p and p->owner... then that's kind of confusing to have a side effect of a method be an unlock.  It may not be possible to refactor that out somehow, but I can see why this has been difficult to maintain.
    
    Also: is there a reason why this was changed from current.chan1 to p->owner?  Since (I think) they're the same thing, and the next comment says not to hold locks on chan1 (being passed to ast_parking_ext_valid), its not intuitively obvious that the unlock of p->owner accomplishes that.
    



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1339/#comment7807>

    See previous comment for consistency between p->owner and current.chan1.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1339/#comment7808>

    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.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1339/#comment7809>

    Remove XXX (unless we have a standard that XXX is 'TODO', or something like that, and we can't fix the TODO under this issue)


- mjordan


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/35c1c08a/attachment.htm>