[asterisk-dev] [Code Review] Deadlock in channel masquerade handling

[David Vossel]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/387/#comment2622>

    This portinuri stuff was not something I added in my branch.  I know what it is, but I don't know why it's in my diff. Please ignore and I will take this out in an update.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/387/#comment2623>

    This trylock loop is unnecessary, I will be replacing it with a simple ast_channel_lock(chan)



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/387/#comment2624>

    ah, I believe ast_channel_release does set clonechan to NULL.  This change is not necessary 


- David


On 2009-09-30 18:36:35, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/387/
> -----------------------------------------------------------
> 
> (Updated 2009-09-30 18:36:35)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In trunk, channels are stored in an ao2_container.  When accessing an item within an ao2_container the proper locking order is to first lock the container, and then the items within it.
> 
> In ast_do_masquerade both the clone and original channel must be locked for the entire duration of the function.  The problem with this is that it attempts to unlink and link these channels back into the ao2_container when one of the channel's name changes.  This is invalid locking order as the process of unlinking and linking will lock the ao2_container while the channels are locked!!! Now, both the channels in do_masquerade are unlinked from the ao2_container and then locked for the entire function.  At the end of the function both channels are unlocked and linked back into the container with their new names as hash values.
> 
> This new method of requiring all channels to be unlocked before ast_do_masquerade or ast_change_name required several changes throughout the code base.  I started by fixing every instance where these two functions were used, and then attempted to spiral out from there verifying no additional channel locks were held outside of the functions that called them.  This was a complex task and I believe I found all the obvious violations of this rule... It is possible by some series of indirection that I may have missed code paths that could again cause a problem.
> 
> 
> This addresses bug 15911.
>     https://issues.asterisk.org/view.php?id=15911
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_misdn.c 221484 
>   /trunk/channels/chan_sip.c 221484 
>   /trunk/include/asterisk/channel.h 221484 
>   /trunk/main/channel.c 221484 
>   /trunk/main/features.c 221484 
>   /trunk/main/pbx.c 221484 
> 
> Diff: https://reviewboard.asterisk.org/r/387/diff
> 
> 
> Testing
> -------
> 
> I completed an attended transfer in chan_sip.
> 
> 
> Thanks,
> 
> David
> 
>