[asterisk-dev] [Code Review] 2946: Fix lock inversion between channels and bridges

[rmudgett]

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



branches/12/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2946/#comment19217>

    Calling the native_rtp_bridge_stop() and native_rtp_bridge_start() functions without the bridge locked is bad.  You should never access the bridge channels list without the bridge locked.



branches/12/channels/chan_pjsip.c
<https://reviewboard.asterisk.org/r/2946/#comment19218>

    I think this is the wrong place to lock the channel.  This function should be called with the channel already locked so other channel drivers will have similar locked environments in their versions of the function.
    
    The function is called with the channel already locked on early media.  The function is called by native_rtp_bridge_start()/native_rtp_bridge_stop() with the bridge locked but not the channel.



branches/12/main/channel.c
<https://reviewboard.asterisk.org/r/2946/#comment19219>

    Add the note
    
     * \note Absolutely _NO_ channel locks should be held before calling this function.
    
    to the ast_channel_bridge_peer() doxygen comment in channel.h.


- rmudgett


On Oct. 22, 2013, 3:21 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2946/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 3:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22628
>     https://issues.asterisk.org/jira/browse/ASTERISK-22628
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This corrects two locking inversions between channels and bridges. The proper locking order is bridge -> bridge_channel -> channel.
> 
> The instance in ast_do_masquerade is easy enough to spot and fix.
> 
> The other instance involves framehooks which are always called with the channel locked and must not be unlocked, a native RTP bridge setup, and a chan_pjsip channel tech callback that attempts to lock a channel's associated bridge. Fortunately, it is not actually necessary to lock the bridge in question and reach across to find the bridged peer since a check to see if the associated bridge exists will do just as well.
> 
> This also cleans up some unnecessary locking in bridge_native_rtp.c.
> 
> 
> Diffs
> -----
> 
>   branches/12/bridges/bridge_native_rtp.c 401310 
>   branches/12/channels/chan_pjsip.c 401310 
>   branches/12/main/channel.c 401310 
> 
> Diff: https://reviewboard.asterisk.org/r/2946/diff/
> 
> 
> Testing
> -------
> 
> This resolves a deadlock that sometimes occurs when attempting to create a 4-way call via attended transfers.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131022/638907e3/attachment-0001.html>