<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2915/">https://reviewboard.asterisk.org/r/2915/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Mark</p>
<br />
<p>On October 16th, 2013, 3:58 p.m. UTC, rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Oct. 16, 2013, 3:58 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22542">ASTERISK-22542</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/apps/app_dial.c <span style="color: grey">(401095)</span></li>
<li>/branches/12/include/asterisk/channel.h <span style="color: grey">(401095)</span></li>
<li>/branches/12/main/channel.c <span style="color: grey">(401095)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2915/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>