[asterisk-dev] [Code Review] app_alarmreceiver cleanup and fixes

[Mark Michelson]

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



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2075/#comment13533>

    There is a potential to write past the end of the digit_string buffer here. If i == length, then you'll break. At this point, you'll attempt to write the null byte past the end of the buffer.
    
    Either change the if statement to break if i == length - 1 or change the assignment of the null byte to ensure that you do not write past the end of the digit string.



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2075/#comment13535>

    There are a few things I don't like here.
    
    First, I think you should pass the size of the event as a parameter to this function. This way there are fewer magic 16s being used in this function. I had to look a few times to make sure you could not possibly access data you should not be accessing.
    
    Second, you can declare the arrays without the size being specified (e.g. static char digit_map[] = "0123456789*#ABC";)
    
    Third, instead of using sizeof(digit_map) use ARRAY_LEN(digit_map). In this case, they evaluate to the same thing, but using ARRAY_LEN makes it much clearer that you mean the length of the array.
    


- Mark


On Aug. 17, 2012, 1:17 p.m., KNK wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2075/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2012, 1:17 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Review on behalf of Pedro Kiefer: cleanups from ASTERISK-20224, ASTERISK-20157 and the updated ASTERISK-16694 (author Fred van Lieshout) + improvement from ASTERISK-20158
> There are several issues opened for bugs in app_alarmreceiver (ASTERISK-19435, ASTERISK-18417, ASTERISK-16694 and ASTERISK-16668), which go away after the cleanup (ASTERISK-16694) replacing the proprietary toneburst code with ast_playtones_ functions, so the fix from ASTERISK-19435 is no longer necessary.
> The new functionality from ASTERISK-20158 is a bugfix on my opinion, because 'we get stuck on the alarm receiver app until it times out'
> 
> 
> This addresses bugs ASTERISK-16668, ASTERISK-16694, ASTERISK-18417, ASTERISK-19435, ASTERISK-20157, ASTERISK-20158, and ASTERISK-20224.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16668
>     https://issues.asterisk.org/jira/browse/ASTERISK-16694
>     https://issues.asterisk.org/jira/browse/ASTERISK-18417
>     https://issues.asterisk.org/jira/browse/ASTERISK-19435
>     https://issues.asterisk.org/jira/browse/ASTERISK-20157
>     https://issues.asterisk.org/jira/browse/ASTERISK-20158
>     https://issues.asterisk.org/jira/browse/ASTERISK-20224
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_alarmreceiver.c 370760 
> 
> Diff: https://reviewboard.asterisk.org/r/2075/diff
> 
> 
> Testing
> -------
> 
> Compile test only
> 
> 
> Thanks,
> 
> KNK
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120820/2d7e8c95/attachment-0001.htm>