<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/3356/">https://reviewboard.asterisk.org/r/3356/</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 14th, 2014, 1:18 p.m. CDT, <b>rmudgett</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/3356/diff/1/?file=56016#file56016line624" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/callerid.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; ">int callerid_feed(struct callerid_state *cid, unsigned char *ubuf, int len, format_t codec)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">624</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="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">b</span> <span class="o"><span class="hl">!=</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="mi"><span class="hl">256</span></span><span class="hl"> </span><span class="o"><span class="hl">-</span></span> <span class="p">(</span><span class="n">cid</span><span class="o">-></span><span class="n">cksum</span> <span class="o">&</span> <span class="mh">0xff</span><span class="p">))<span class="hl">)</span></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">624</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="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">b</span> <span class="o"><span class="hl">+</span></span> <span class="p">(</span><span class="n">cid</span><span class="o">-></span><span class="n">cksum</span> <span class="o">&</span> <span class="mh">0xff</span><span class="p">))</span> <span class="p">{</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;">I would be surprised if this worked for calls that the checksum was not zero.
I think the expression should be:
if ((b + cid->cksum) & 0xff)
Otherwise you have a more likely overflow when the message bytes sum to larger than 8 bits.</pre>
</blockquote>
<p>On March 14th, 2014, 1:57 p.m. CDT, <b>rmeyerriecks</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;">The "b" variable already has prior logic that decides to mask its lower 8 bits or not. I did it this way to maintain CID failure in the case of bit 8 or bit 9 being set by fsk_modem. Your line would ignore both those errors.
Granted, I could see the argument that we should probably try to pass CID even in the case that we get checksum errors, but I was just trying to follow the code convention.</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;">Actually, although I didn't see this originally, Richard is correct. If you have for example b = 0x7f added to checksum&0xff = 0x81, the result is 0x100 which is going to fail the checksum test when it should pass. Even though fsk_serial could potentially return a b value > 0xff, that should be a separate check for an invalid character (which realistically would require a software bug in the uart emulation to happen) rather than built into the crc check.
I recommend one of these alternatives in addition to Richard's valid approach:
if (b + (cid->cksum & 0xff) == 0x100)
if (b == (~cid->cksum + 1) & 0xff)
The other option would be to do:
cid->cksum += b;
if (cid->cksum & 0xff)
</pre>
<br />
<p>- Scott</p>
<br />
<p>On March 14th, 2014, 12:10 p.m. CDT, rmeyerriecks 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 rmeyerriecks.</div>
<p style="color: grey;"><i>Updated March 14, 2014, 12:10 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-23488">asterisk-23488</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;">Callerid checksum-ing is being handled incorrectly here. When the checksum is calculated to be 0x00, it will perform 0x100-0x00 which results in 0x100. This value will then fail the otherwise correct callerid message. It was intended for devices on the rx side to simply add the calculated checksum to the transmitted 2's compliment checksum. A much simpler operation.</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/main/callerid.c <span style="color: grey">(410575)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3356/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>