[asterisk-dev] [Code Review]: Fix potential deadlock between masquerade and chan_local.

[rmudgett]

> On May 11, 2012, 3:05 p.m., Mark Michelson wrote:
> > /branches/1.8/main/channel.c, line 6540
> > <https://reviewboard.asterisk.org/r/1915/diff/1/?file=27808#file27808line6540>
> >
> >     s/indiction/indication/

done


> On May 11, 2012, 3:05 p.m., Mark Michelson wrote:
> > /branches/1.8/main/channel.c, lines 6941-6955
> > <https://reviewboard.asterisk.org/r/1915/diff/1/?file=27808#file27808line6941>
> >
> >     I'll admit I'm not 100% in it here, but it seems like it's possible to unref the clonechan too many times if clone_was_zombie.

The original code called ast_channel_release() instead of simply unrefing the channel.  ast_channel_release() just unlinks and unrefs the channel.  Since the channel is already unlinked because of the masquerade channel renaming, we only need to reduce the channel ref.  Since ast_do_masquerade() now increases the channel refs of the original and clonechan at the beginning for safety, the channel will be destroyed at the end of the routine when the safety refs are released.

I don't think the safety refs are really needed but it doesn't hurt.


- rmudgett


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


On May 11, 2012, 12:05 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1915/
> -----------------------------------------------------------
> 
> (Updated May 11, 2012, 12:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ast_do_masquerade() calls ast_indicate() with the channel lock held.  This is a deadlock waiting to happen if the channel involved is a local channel.
> 
> * Restructure ast_do_masquerade() to not hold channel locks while it calls ast_indicate().
> 
> * Simplify many calls to ast_do_masquerade() since it will never return a failure now.  If it does fail internally because a channel driver callback operation failed, the only thing ast_do_masquerade() can do is generate a warning message about strange things may happen and press on.
> 
> * Fixed the call to ast_bridged_channel() in ast_do_masquerade().  This change fixes half of the deadlock reported in ASTERISK-19801 between masquerades and chan_iax.
> 
> 
> This addresses bugs ASTERISK-19537 and ASTERISK-19801.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19537
>     https://issues.asterisk.org/jira/browse/ASTERISK-19801
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/channel.c 366290 
> 
> Diff: https://reviewboard.asterisk.org/r/1915/diff
> 
> 
> Testing
> -------
> 
> A manual version of the yet to be written external test in ASTERISK-19807 passes.  I tested this patch with a call having a chain of 300 local channel pairs that optimize themselves out of the call.  It takes a minute for the local channels to get optimized out but once they are gone the call passes audio.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120512/aa355edf/attachment-0001.htm>