[asterisk-dev] [Code Review]: unchecked_return audit and fixes

[Tilghman Lesher]

> On May 8, 2012, 1:55 p.m., Tilghman Lesher wrote:
> > /branches/1.8/main/asterisk.c, lines 3227-3230
> > <https://reviewboard.asterisk.org/r/1905/diff/1/?file=27754#file27754line3227>
> >
> >     Adding this error message here will cause the log to be continuously spammed as fast as the program can run.  Remember, this is inside an infinite loop, so if the file is deleted, you're just going to keep pumping out this error message without ever actually handling the condition.  Not good.
> 
> jrose wrote:
>     Maybe I should copy the 60 second sleep before continuing then?  It seems like I can't really just keep going through without risking using a canary_stat filled with garbage at this point anyway... which could probably result in prematurely killing the Canary.
>     
>     But yeah, you are right, just spitting the error out over and over again in rapid succession is a little ridiculous.

Since that file is crucial to the priority checking code, deleting it or making it otherwise inaccessible should immediately reduce the priority to normal.


> On May 8, 2012, 1:55 p.m., Tilghman Lesher wrote:
> > /branches/1.8/main/pbx.c, line 6544
> > <https://reviewboard.asterisk.org/r/1905/diff/1/?file=27757#file27757line6544>
> >
> >     Is it really necessary to put the tool name in a comment?  This sort of feels like comments that include someone's initials, which are contrary to coding guidelines.
> 
> jrose wrote:
>     Well, this is part of an ongoing effort to clear out warnings generated by Coverity. There is supposed to be a specific way of flagging them to be ignored by future scans, but the exact means of doing so is eluding us right now, so this is sort of a reminder to do that when we get ahold of the means to do so.

How about just /* Ignore check_return warning for ast_exists_extension */ ?


> On May 8, 2012, 1:55 p.m., Tilghman Lesher wrote:
> > /branches/1.8/res/ael/ael.flex, lines 878-884
> > <https://reviewboard.asterisk.org/r/1905/diff/1/?file=27759#file27759line878>
> >
> >     Now that I see what this code does, it'd probably be better if this stat were simply replaced with two lseek()s, instead (which is much faster).  The same is true for the earlier case in this file.
> 
> jrose wrote:
>     Yeah, that's true, but optimization isn't really the purpose of this issue.  I'm wouldn't be averse to doing this if ael_parse was something that had to be executed with high frequency, but I don't really think it is.
> 
> jrose wrote:
>     s/I'm wouldn't/I wouldn't/

You should probably return, then.  If stat() fails, then you're not going to have the size for the malloc, so the code will read in essentially a zero-length file.


- Tilghman


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


On May 8, 2012, 3:03 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1905/
> -----------------------------------------------------------
> 
> (Updated May 8, 2012, 3:03 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Makes corrections for a number of unchecked return reports from Coverity and marks a handful of others as being ignorable (because the return seems genuinely inconsequential for these cases).  There are a couple oddities that might need a second look.
> 
> 
> This addresses bug ASTERISK-19658.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19658
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_queue.c 365473 
>   /branches/1.8/apps/app_voicemail.c 365473 
>   /branches/1.8/channels/chan_iax2.c 365473 
>   /branches/1.8/channels/chan_sip.c 365473 
>   /branches/1.8/channels/iax2-provision.c 365473 
>   /branches/1.8/channels/sig_analog.c 365473 
>   /branches/1.8/funcs/func_devstate.c 365473 
>   /branches/1.8/funcs/func_lock.c 365473 
>   /branches/1.8/main/acl.c 365473 
>   /branches/1.8/main/asterisk.c 365473 
>   /branches/1.8/main/db.c 365473 
>   /branches/1.8/main/features.c 365473 
>   /branches/1.8/main/pbx.c 365473 
>   /branches/1.8/main/xmldoc.c 365473 
>   /branches/1.8/res/ael/ael.flex 365473 
> 
> Diff: https://reviewboard.asterisk.org/r/1905/diff
> 
> 
> Testing
> -------
> 
> Testing done depends on the particular issue that was being poked at. In some cases, it was as simple as a snap judgement to add a warning. In a couple others, I would test what would happen as a consequence of an unanticipated return.
> 
> 
> Thanks,
> 
> jrose
> 
>

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