[asterisk-dev] [Code Review] 3356: Logic error in callerid checksum processing

[Scott Griepentrog]

> On March 14, 2014, 1:18 p.m., rmudgett wrote:
> > /branches/1.8/main/callerid.c, line 624
> > <https://reviewboard.asterisk.org/r/3356/diff/1/?file=56016#file56016line624>
> >
> >     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.
> 
> rmeyerriecks wrote:
>     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.

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)


- Scott


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3356/#review11208
-----------------------------------------------------------


On March 14, 2014, 12:10 p.m., rmeyerriecks wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3356/
> -----------------------------------------------------------
> 
> (Updated March 14, 2014, 12:10 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: asterisk-23488
>     https://issues.asterisk.org/jira/browse/asterisk-23488
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/callerid.c 410575 
> 
> Diff: https://reviewboard.asterisk.org/r/3356/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> rmeyerriecks
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140314/8abe3938/attachment-0001.html>