<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/1863/">https://reviewboard.asterisk.org/r/1863/</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/1863/diff/2/?file=27199#file27199line723" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/translate.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 *handle_cli_core_show_translation(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)</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">723</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">dst</span> <span class="o"><</span> <span class="mi">0</span> <span class="o">||</span> <span class="n">src</span> <span class="o"><</span> <span class="mi">0</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">724</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_cli</span><span class="p">(</span><span class="n">a</span><span class="o">-></span><span class="n">fd</span><span class="p">,</span> <span class="s">"Failed to determine translation path for %s codec %s - skipping</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">dst</span> <span class="o"><</span> <span class="mi">0</span> <span class="o">?</span> <span class="s">"destination"</span> <span class="o">:</span> <span class="s">"source"</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">725</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">dst</span> <span class="o"><</span> <span class="mi">0</span> <span class="o">?</span> <span class="n">format_list</span><span class="p">[</span><span class="n">i</span><span class="p">].</span><span class="n">name</span> <span class="o">:</span> <span class="n">a</span><span class="o">-></span><span class="n">argv</span><span class="p">[</span><span class="mi">4</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">726</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">continue</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">727</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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;">Yes, the way the code is currently written the inputs are sanitized or come from our pre-built format list, so powerof cannot return -1.
However, I don't like the idea of writing another powerof function - what should it return if no bits are set? If it is supposed to just never return -1, then we should just use ffsll and be done with it - in fact, all powerof does for you is guarantee that you get a WARNING if no bits were set - which is useful.
As it is, there isn't a way to prevent a static analysis tool from complaining about this. Using ffsll(x)-1 would still trigger a complaint about a potential negative index into an array, and a function that returned 0 instead of -1 would semantically be wrong, as it would imply the first bit was set when it wasn't.
I'd either prefer to leave the code in here, or remove it and put a comment stating that it is expected that powerof cannot return -1 in this case. If the code ever changes and that assumption is false, at least we stated it in the code.</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/1863/diff/2/?file=27199#file27199line1093" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/translate.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">unsigned int ast_translate_path_steps(format_t dest, format_t src)</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">1093</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">src_audio</span> <span class="o"><</span> <span class="mi">0</span> <span class="o">||</span> <span class="n">src_video</span> <span class="o"><</span> <span class="mi">0</span><span class="p">)</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;">Yes, if src is 0 we return the destination format.
Again, I'd either prefer to leave this here in case someone removes that portion of this method, or simply place a comment that says src_audio/src_video cannot be less than 0.</pre>
</div>
<br />
<p>- Matt</p>
<br />
<p>On April 11th, 2012, 3:52 p.m., Matt Jordan 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 Matt Jordan.</div>
<p style="color: grey;"><i>Updated April 11, 2012, 3:52 p.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;">Through a static analysis tool, a large number of errors were found that dealt with using potential negative return values in harmful ways. The following is a summary of the changes for the various affected files:
* app_voicemail
- a negative result from lseek is later passed into mmap as the size of the memory map
- a negative result from read closes the various file descriptors and unlinks the output file, but the next check against the return value only checks to see if its non-zero
* chan_agent - ast_channel_unlock returns a negative value if the item passed to it is NULL or an invalid ao2 object. strerror cannot accept a negative value.
* chan_dahdi - if dahdi_get_index returns a negative value, we index directly into an array
* format_* - various errors not checking return values of ftello and fseek, and passing the results into functions that expect non-negative values
* func_env - various places where the result of ftello was directly used as input to functions that expect non-negative values
* asterisk
- the request to read returns a negative value. This causes us to enter into the retry logic. If we attempt to reconnect and succeed, we would normally proceed in the for(;;) loop and attempt to index into buf using the negative return value from read. Instead, if we do reconnect, we immediately return to the beginning of the for(;;) loop and attempt a new read.
- similar situation - if a read fails, don't attempt to index into a buffer using the return value
* frame - if we can't determine a preferred codec using the provided values, don't attempt to use an index value that never got set
* manager
- various failures of mkstemp, lseek were not checked for and could be provided to methods that don't handle negative numbers
- passing a negative result from lseek into mmap as the size of the memory map
* translate - powerof can return a negative result if no bits are set, which would then be used as an index into an array
* res_agi - if read returns an error, we treated it as if bytes were read from the pipe
* res_musiconhold - if we fail to spawn spawn_mp3 returns a negative number, we wait a bit and attempt again later. However, the return 'file descriptor' srcfd is later passed into read.
* res_rtp_asterisk - ast_rtp_codecs_payload_code can return a negative value if no codecs are found that match between instance1 and the specified payload. If that's the case, the bridge should be broken, as there are no compatible formats between the two different endpoints (and we shouldn't index into an array using the return value)</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/1.8/res/res_agi.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/main/translate.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/main/asterisk.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/main/frame.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/main/manager.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/funcs/func_env.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_wav_gsm.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_wav.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_sln16.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_vox.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_siren7.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_sln.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_siren14.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_pcm.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_h264.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_ilbc.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_h263.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_g729.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_gsm.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_g719.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/formats/format_g723.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/channels/chan_agent.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/channels/chan_dahdi.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/apps/app_voicemail.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/res/res_musiconhold.c <span style="color: grey">(361945)</span></li>
<li>/branches/1.8/res/res_rtp_asterisk.c <span style="color: grey">(361945)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1863/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>