<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/2620/">https://reviewboard.asterisk.org/r/2620/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2620/diff/4-5/?file=39828#file39828line516" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/bridges/bridge_builtin_features.c</a>
<span style="font-weight: normal;">
(Diff revisions 4 - 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">516</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">c_touch_format</span> <span class="o">=</span> <span class="n">pbx_builtin_getvar_helper</span><span class="p">(</span><span class="n">chan</span><span class="p">,</span> <span class="n">is_mixmonitor</span> <span class="o">?</span> <span class="s">"TOUCH_MIXMONITOR_FORMAT"</span> <span class="o">:</span> <span class="s">"TOUCH_MONITOR_FORMAT"</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">517</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="o">*</span><span class="n">touch_format</span> <span class="o">=</span> <span class="n">ast_strdup</span><span class="p">(</span><span class="n">S_OR</span><span class="p">(</span><span class="n">c_touch_format</span><span class="p">,</span> <span class="s">""</span><span class="p">));</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">518</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">519</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">ast_strlen_zero</span><span class="p">(</span><span class="o">*</span><span class="n">touch_format</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">520</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="mi">0</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">521</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This idiom, in tandem with how set_touch_variables() is used, may cause a memory leak.
*touch_format is guaranteed to allocate some memory, either copying c_touch_format or copying an empty string. The other touch variables follow the same pattern. If all three touch variables copy an empty string, then it means that set_touch_variables() will return non-zero.
In the places where set_touch_variables() is used, if the return is non-zero, then set_touch_variables() is called a second time with the same pointers passed in. This will result in the original allocations of empty strings being overwritten. The second allocation will be properly freed since the pointers are RAII_VARs.
There are a few possible fixes:
1) Change the ast_strdup() to be
if (!ast_strlen_zero(c_touch_format)) {
*touch_format = ast_strdup(c_touch_format);
res = 0;
}
This way, there is a direct correlation between duplicating a valid string and returning 0.
2) Have distinct sets of pointers to pass into set_touch_variables() so that you don't overwrite allocations
3) Explicitly free the pointers after the first call to set_touch_variables() before calling it a second time.</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On June 26th, 2013, 8:50 p.m. UTC, jrose 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, Matt Jordan and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated June 26, 2013, 8:50 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-21553">asterisk-21553</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;">Adds Monitor and MixMonitor as bridge features. In order to avoid using PBX execution and autoservice, I added some additional API for MixMonitor provided through loadable function callbacks provided by app_mixmonitor.
In order to make One Touch MixMonitor behave more like its Monitor equivalent, I went ahead and added a couple channel variables for playing start and stop recording sounds (TOUCH_MIXMONITOR_MESSAGE_START and TOUCH_MIXMONITOR_MESSAGE_STOP)</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;">Tried both Monitor and MixMonitor with various channel variables set for filename/format and start/stop sounds. They appear to be working as one would expect.
Also checked the behavior of MixMonitor with app_mixmonitor loaded and unloaded. As expected, it is rejected smoothly when MixMonitor is not loaded.</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>/trunk/CHANGES <span style="color: grey">(392970)</span></li>
<li>/trunk/apps/app_mixmonitor.c <span style="color: grey">(392970)</span></li>
<li>/trunk/bridges/bridge_builtin_features.c <span style="color: grey">(392970)</span></li>
<li>/trunk/configs/features.conf.sample <span style="color: grey">(392970)</span></li>
<li>/trunk/include/asterisk/features_config.h <span style="color: grey">(392970)</span></li>
<li>/trunk/include/asterisk/mixmonitor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/features_config.c <span style="color: grey">(392970)</span></li>
<li>/trunk/main/mixmonitor.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2620/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>