<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/1643/">https://reviewboard.asterisk.org/r/1643/</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/1643/diff/5/?file=23193#file23193line591" style="color: black; font-weight: bold; text-decoration: underline;">http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel *chan)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel *chan, char *datastore_id)</pre></td>
</tr>
</tbody>
<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">591</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">char</span> <span class="n">datastore_id</span><span class="p">[</span><span class="mi">8</span><span class="p">]</span> <span class="o">=</span> <span class="s">""</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;">You have a rather major faux-pas here that I noticed while testing. You are using this as a string of 8 characters, but you have no place for a termination character in that string. When sprintf writes to this, it attempts to place a terminator string at index 8 which gets caught by overflow protection making Asterisk crash.
I think this should be fairly easy to fix just by making this a 9 character buffer.</pre>
</div>
<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/1643/diff/5/?file=23193#file23193line814" style="color: black; font-weight: bold; text-decoration: underline;">http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static char *filename_parse(char *filename, char *buffer, size_t len)</pre></td>
</tr>
</tbody>
<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">813</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">mixmonitor_ds</span><span class="o">-></span><span class="n">audiohook</span> <span class="o">&&</span> <span class="n">mixmonitor_ds</span><span class="o">-></span><span class="n">audiohook</span><span class="o">-></span><span class="n">status</span> <span class="o">!=</span> <span class="n">AST_AUDIOHOOK_STATUS_DONE</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">814</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_audiohook_update_status</span><span class="p">(</span><span class="n">mixmonitor_ds</span><span class="o">-></span><span class="n">audiohook</span><span class="p">,</span> <span class="n">AST_AUDIOHOOK_STATUS_SHUTDOWN</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;">All if statements should use braces regardless of number of lines. I know this isn't actually realized everywhere, but it's in the style guide and we enforce it for all new code.</pre>
</div>
<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;">By the by, do you have commit access? If not, I can take it from here.</pre>
<p>- jrose</p>
<br />
<p>On January 17th, 2012, 5:26 a.m., telecos82 wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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 telecos82.</div>
<p style="color: grey;"><i>Updated Jan. 17, 2012, 5:26 a.m.</i></p>
<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;">Currently, if there are several MixMonitor started on a channel, there is no way to specify which MixMonitor to stop with StopMixMonitor. With this patch we will allow this. One limitation of current implementation was that mixmonitor datastore was not identified with a uid so there was no way to identify a concrete MixMonitor. Furthermore, when retrieving audiohook to dettach from channel, there was no control on which audiohook to dettach, it always got first audiohook found of type mixmonitor_spy_type. With this patch we are identifying MixMonitor datastore by the filename asociated to it, and dettaching the concrete audiohook contained on datastore. Furthermore a new CLI command "mixmonitor list <channel_name>" to help querying active mixmonitors on a channel.</pre>
</td>
</tr>
</table>
<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-19096">ASTERISK-19096</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c <span style="color: grey">(348737)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1643/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>