[asterisk-dev] [Code Review] 2915: Add channel lock protection around translation path setup.

[Mark Michelson]

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


I made a small effort to look at whether this added locking might cause issues. First file I checked was res_fax.c. If you look at the fax_gateway_framehook(), you'll find that chan is locked, and then formats are set on both the chan and peer channels (line ~3113 in the 12 branch). Setting the read and write formats on the peer at this point now introduces a potential deadlock.

I think that an audit of the potential effects of this change is required. One of two things needs to be done based on this audit.

1) If it turns out I was unlucky and discovered the one and only potential deadlock spot in the code, or if there are only a couple of places like it, then those places need to be rearranged to prevent a potential deadlock.
2) If this is a more widespread a problem, then you should not embed the locking down in the channel.c code but rather require that callers of the functions grab the lock themselves before calling.

Whichever way this goes, the affected public functions in channel.h need to be documented as either requiring a lock be grabbed before calling or as grabbing locks itself so that the implications are clear.

- Mark Michelson


On Oct. 16, 2013, 3:58 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2915/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2013, 3:58 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22542
>     https://issues.asterisk.org/jira/browse/ASTERISK-22542
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Most callers of ast_channel_make_compatible() happen before the channels enter a two party bridge.  With the new bridging framework, two party bridging technologies may also call ast_channel_make_compatible() when there is more than one thread involved with the two channels.
> 
> * Added channel lock protection in set_format() and ast_channel_make_compatible_helper() when dealing with the channel's native formats while setting up a translation path.
> 
> * Fixed best_src_fmt and best_dst_fmt usage consistency in ast_channel_make_compatible_helper().  The call to ast_translator_best_choice() got them backwards.
> 
> * Updated some callers of ast_channel_make_compatible() and the function documentation.  There is actually a difference between the two channels passed in.
> 
> 
> Diffs
> -----
> 
>   /branches/12/apps/app_dial.c 401095 
>   /branches/12/include/asterisk/channel.h 401095 
>   /branches/12/main/channel.c 401095 
> 
> Diff: https://reviewboard.asterisk.org/r/2915/diff/
> 
> 
> Testing
> -------
> 
> Since there is not enough information on ASTERISK-22542 to determine why no translation path could happen, the best that testing can do is show that the change does not cause problems.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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