<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/4526/">https://reviewboard.asterisk.org/r/4526/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 26th, 2015, 6:37 p.m. CET, <b>Diederik de Groot</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/4526/diff/1/?file=72920#file72920line728" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/funcs/func_env.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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 int file_read(struct ast_channel *chan, const char *cmd, char *data, struct ast_str **buf, ssize_t len)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">728</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ast_str_append_substr</span><span class="p">(</span><span class="n">buf</span><span class="p">,</span> <span class="n">len</span><span class="p">,</span> <span class="n">fbuf</span><span class="p">,</span> <span class="n">length_offset</span> <span class="o">>=</span> <span class="mi">0</span> <span class="o">?</span> <span class="n">length_offset</span> <span class="o">-</span> <span class="n">i</span> <span class="o">:</span> <span class="n">flength</span> <span class="o">></span> <span class="n">i</span> <span class="o">+</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">fbuf</span><span class="p">))</span> <span class="o">?</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">fbuf</span><span class="p">)</span> <span class="o">:</span> <span class="n">flength</span> <span class="o">-</span> <span class="n">i</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">728</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ast_str_append_substr</span><span class="p">(</span><span class="n">buf</span><span class="p">,</span> <span class="n">len</span><span class="p">,</span> <span class="n">fbuf</span><span class="p">,</span> <span class="p"><span class="hl">(</span></span><span class="n">length_offset</span> <span class="o">>=</span> <span class="mi">0</span><span class="p"><span class="hl">)</span></span> <span class="o">?</span> <span class="n">length_offset</span> <span class="o">-</span> <span class="n">i</span> <span class="o">:</span> <span class="p"><span class="hl">(</span></span><span class="n">flength</span> <span class="o">></span> <span class="n">i</span> <span class="o">+</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">fbuf</span><span class="p">))</span> <span class="o">?</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">fbuf</span><span class="p">)</span> <span class="o">:</span> <span class="n">flength</span> <span class="o">-</span> <span class="n">i</span><span class="p"><span class="hl">)</span>;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Note the double closing parens after the first sizeof, i actually finishes the ast_str_append_substr command and then continues with the second sizeof.
Not a 100% sure if this is corrected in the right way, but the original doesn't look right either.
</pre>
</blockquote>
<p>On March 27th, 2015, 2:02 a.m. CET, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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'd double check that the unit tests still pass with the change.</pre>
</blockquote>
<p>On March 27th, 2015, 12:11 p.m. CET, <b>Diederik de Groot</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Currently not running any tests, just making the changes for now. Sounds silly, but it is enough work as it is. Automated testing should spit out any issues later on i hope.</pre>
</blockquote>
<p>On March 27th, 2015, 2:18 p.m. CET, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, that would mean someone else would have to go fix it. :-P
It's really easy to run a single unit test to double check that the logic still works - particularly when there's already a unit test written for a particular module. To do that, you:
* Configure Asterisk to enable dev mode: ./configure --enable-dev-mode
* Enable TEST_FRAMEWORK in menuselect's build options
* On the CLI, execute 'test execute category /funcs/func_env/'
Granted, since Asterisk won't compile in dev mode with clang (as WARNINGs are promoted to ERRORs), you'll need to double check it compiled with gcc, but for things like this where stuff is questionable, it doesn't hurt to make sure that the existing tests pass locally before things get committed.</pre>
</blockquote>
<p>On March 27th, 2015, 3:13 p.m. CET, <b>Diederik de Groot</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Now that i am finished i can run a couple of these, i guess. Will give it a go, after my CPU cools down a bit ;-P</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done:
Test passes both with and without the patch, both under gcc compiled asterisk as well as clang compiled asterisk: Result PASS.
SideNote1: All this work i have been doing was to make asterisk compilable clang, so it should be able to run these tests as well, right. With all the submitted patches in place it compiles and runs in dev-mode as well.
SideNote2: Because of the discovered error in this function i guess the test should / would have to be updated for this particular case to prevent future regressions.</pre>
<br />
<p>- Diederik</p>
<br />
<p>On March 27th, 2015, 12:09 p.m. CET, Diederik de Groot 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Diederik de Groot.</div>
<p style="color: grey;"><i>Updated March 27, 2015, 12:09 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-24917">ASTERISK-24917</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;">clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs
clang compiler warning:-Wunused-value -Wunused-variable -Wunused-const-variable
Changes:
apps/app_queue.c: removed unused qpm_cmd_usage[], qum_cmd_usage[], qsmp_cmd_usage[]
cel/cel_sqlite3_custom.c: removed unused name[] = "cel_sqlite3_custom"
channels/chan_pjsip.c: removed unused desc[] = "PJSIP Channel"
codecs/gsm/src/gsm_create.c: removed unused ident[] = "$Header$"
funcs/func_env.c:729: Fixed ast_str_append_substr. Needs to be reviewed by code owner.
main/config.c: inline functions have to be static for clang to grok them
main/editline/np/strlcat.c: removed unused rcsid = "$OpenBSD: strlcat.c,v 1.2 1999/06/17 16:28:58 millert Exp $"
main/editline/np/strlcpy.c: removed unused rcsid = "$OpenBSD: strlcpy.c,v 1.4 1999/05/01 18:56:41 millert Exp $"
main/features.c: removed unused channel_app_data_datastore struct
main/security_events.c: removed unused TIMESTAMP_STR_LEN
utils/conf2ael.c: removed unused cfextension_states
utils/extconf.c: removed unused cfextension_states</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/13/utils/extconf.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/utils/conf2ael.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/security_events.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/features.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/editline/np/strlcpy.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/editline/np/strlcat.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/config.c <span style="color: grey">(433470)</span></li>
<li>/branches/13/funcs/func_env.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/codecs/gsm/src/gsm_create.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/channels/chan_pjsip.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/cel/cel_sqlite3_custom.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/apps/app_queue.c <span style="color: grey">(433444)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4526/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>