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

[rmudgett]

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


The locking here hurts my head.  Oh the pain...


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

    The error exits here fail to unref sip_pvt_ptr.



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

    Uh oh... deadlock potential
    peerb is locked, peerc is not locked
    
    ast_channel_masquerade() is going to do deadlock avoidance to get peerc lock.



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

    The if is not needed.  Just unref it.



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

    We really need an
    ast_bridged_channel_ref()
    that returns a reference to the bridged channel.  The legacy ast_bridged_channel() can then be made to call the new function and remove the reference on the returned channel.
    
    The ast_bridged_channel() should then be deprecated as risky to use!



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

    We have no references to x.chan2 channels so when we release the locks on x.chan1 channels, x.chan2 is not guaranteed to be valid anymore!



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

    Same here just unref it.  targetcall_pvt cannot be NULL.



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

    Make this a doxygen comment for the variable.
    /*!
     * current.chan1: ...
     * current.chan2: ...
     */



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

    Calling get_refer_info() with p and p->owner locked is probably not good.
    
    get_refer_info() calls pbx_builtin_setvar_helper() on the bridged peer of p and thus would need deadlock avoidance between p->owner and peer.



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

    local_attended_transfer() calls get_sip_pvt_byid_locked().  Since p and p->owner are already locked, we can have a deadlock when get_sip_pvt_byid_locked() attempts to lock the referred dialog.  The deadlock potential is reduced by the deadlock avoidance done by get_sip_pvt_byid_locked() but not entirely eliminated since the locks already held could be wanted by some other transfer request.
    
    Reference leak in get_sip_pvt_byid_locked()!
    The sip_pvt_ptr is not unreffed for error exits it is only unlocked.



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

    Typo:
    can no safely
    should be
    cannot safely



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

    Is the note a result of a deadlock seen?
    
    If it is then a similar deadlock interaction with conlock and channels could occur in get_refer_info() when it calls ast_exists_extension().



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

    The XXX note here is not needed.
    
    If you already hold the channel lock to the channel passed to pbx_builtin_setvar_helper() then you are fine.



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

    Swap the pvt unlock and comment here since that is why the comment is here and it is what the comment is saying to do.


- rmudgett


On Aug. 2, 2011, 4:28 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, 4:28 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 330671 
> 
> 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/20110811/18425cba/attachment-0001.htm>