[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 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.
> 
> KNK wrote:
>     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.
> 
> Tilghman Lesher wrote:
>     That seems counterintuitive and it's contrary to how we pass buffer sizes in other areas of the application.  For consistency sake, the passed size should be the total size of the buffer, and we should do any math necessary to ensure we don't overflow that size inside the function.

I think the comment about length here is misleading and should say "Length of the message we expect" instead of 'length of the sting', because it is not the 'size of the buffer' at all, but the 'size of the event message', while the buffer is guaranteed to be bigger.


- 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/a006c7b0/attachment-0001.htm>