[asterisk-dev] [Code Review] 4342: CHANNEL(peer), chan_iax2, res_fax, SNMP agent: Fix deadlock from reaching across a bridge.

[rmudgett]

> On Jan. 15, 2015, 9:25 a.m., Matt Jordan wrote:
> > /branches/13/channels/chan_iax2.c, lines 1287-1289
> > <https://reviewboard.asterisk.org/r/4342/diff/1/?file=70548#file70548line1287>
> >
> >     This change isn't necessary.
> >     
> >     AST_CHAN_TP_CREATESJITTER is only used by ast_jb_do_usecheck, which is dead code. Nothing calls it any longer.
> >     
> >     Previously, ast_jb_do_usecheck was used by ast_generic_bridge to see if it needed to empty/reset the jitter buffers on the channels during bridging.
> >     
> >     Now, instead of manipulating both channels at the same time, the bridging framework checks to see if a channel wants a jitter buffer using ast_jb_enable_for_channel when the channel goes into the bridge. This is done by looking at ast_channel_jb and adding a framehook, as opposed to the technology itself.
> >     
> >     The notion of using channel technology to determine if channels should have jitter buffers only made sense when the de-jittering of a jittered channel occurred on the other side of the bridge. This was necessary when a channel technology such as DAHDI was bridged with a channel technology that provided jitter. This is no longer the case. Now, each channel can have a jitter buffer placed on the read side of the channel. A DAHDI channel can be safely bridged with a jittery channel, so long as that channel has a jitter buffer on it - the DAHDI channel doesn't care one way or the other, as reading from the jittered channel still provides a constant stream of frames.

I only put it there for consistency and because if a channel "wants" jitter it will create jitter for the same reason.


> On Jan. 15, 2015, 9:25 a.m., Matt Jordan wrote:
> > /branches/13/channels/chan_iax2.c, lines 4235-4286
> > <https://reviewboard.asterisk.org/r/4342/diff/1/?file=70548#file70548line4235>
> >
> >     If we want to force a jitter buffer, we should simply set up a read-side jitter buffer on the channel and be done. The notion of reaching across the bridge to determine if they are "okay" with us setting up a jitter buffer is no longer needed.

Removing this code eliminates the iax.conf forcejitterbuffer option as this was the only place where it was used.  The other places where the flag was referenced were either copying it or setting it from the config file.  Also the code will be faster since the bridge peer check was relatively expensive for every frame.


- rmudgett


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


On Jan. 14, 2015, 4:25 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4342/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24600
>     https://issues.asterisk.org/jira/browse/ASTERISK-24600
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Calling ast_channel_bridge_peer() cannot be done while holding any channel
> locks.  The reported issue hit the deadlock in chan_iax2, but an audit of
> the ast_channel_bridge_peer() calls found three more locations where the
> same deadlock can occur.
> 
> * Made CHANNEL(peer), chan_iax2, res_fax, and the SNMP agent not call
> ast_channel_bridge_peer() with any channel locked.  For CHANNEL(peer) and
> chan_iax2 I had to rework the logic to not hold the channel lock.
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/snmp/agent.c 430625 
>   /branches/13/res/res_fax.c 430625 
>   /branches/13/funcs/func_channel.c 430625 
>   /branches/13/channels/chan_iax2.c 430625 
> 
> Diff: https://reviewboard.asterisk.org/r/4342/diff/
> 
> 
> Testing
> -------
> 
> Testsuite tests still pass for FAX.  The full testsuite didn't hit any deadlock.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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