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

[KNK]

> On Aug. 20, 2012, 1:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_alarmreceiver.c, lines 478-497
> > <https://reviewboard.asterisk.org/r/2075/diff/3/?file=31019#file31019line478>
> >
> >     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.
> >
> 
> rmudgett wrote:
>     You have to declare digit_map[15] = "0123456789*#ABC" because initializing it with a string would also includes a nul terminator in the array size.  Of course you could initialize it character by character.  Then you would not need the explicit array dimension.
>     
>     Another way to declare this so you know what maps with what is:
>     struct {
>       char digit;
>       char weight;
>     } mapping[] = { {'0', 10 },... };
>

The size of the event was planned to be passed to this function in a later patch as different formats have different length and some of them do not have checksums at all, so can we leave this as is for now?

P.S. I like the idea for the mapping structure - will change that too, but in a later patch if it is OK.


> On Aug. 20, 2012, 1:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_alarmreceiver.c, lines 310-318
> > <https://reviewboard.asterisk.org/r/2075/diff/3/?file=31019#file31019line310>
> >
> >     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.

The lenght is passed to this function as "sizeof(event) - 1", so there is no write past the end of the buffer.
When other formats are added (later), they will pass to this function the number of digits they expect and the buffer is always bigger - please let's leave it as is for now.


- KNK


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


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/20120821/870baaf1/attachment-0001.htm>