[asterisk-dev] [Code Review]: Fix a large variety of errors caused by negative return codes

Matt Jordan reviewboard at asterisk.org
Wed Apr 11 15:51:10 CDT 2012 


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/apps/app_voicemail.c, line 3864
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27151#file27151line3864>
> >
> >     fdlen is no longer a size_t so you just need to use %d or change fdlen to ssize_t.

Or just get rid of the debug statement, which added very little value.


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_dahdi.c, lines 8731-8734
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27153#file27153line8731>
> >
> >     This should do the same thing as analog_exception() in sig_analog.c.

I saw that in sig_analog but wasn't sure if the same could safely be done here as well.  Done.


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/formats/format_gsm.c, lines 130-134
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27157#file27157line130>
> >
> >     Since you're formatting here, add a space between if(

Done


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/main/manager.c, line 3634
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27172#file27172line3634>
> >
> >     What about this lseek?

Technically, not covered by this review because we don't use the value that it returns.  Presumably, if that lseek failed then we would fail to set the position to the beginning fo the file.  If that's the case, then the last successful lseek would have placed us at the end of the file.  The subsequent read operation would read 0 bytes (EOF) and would presumably place nothing in the buffer.  Eventually we'd end up trying to append a buffer filled with NULL characters to the manager response - which wouldn't cause a crash, but would probably be incorrect from the perspective of the receiver.

I'll just check for the return value of lseek as well.


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/main/manager.c, lines 3635-3639
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27172#file27172line3635>
> >
> >     buf is leaked here.

Done


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/res/res_agi.c, line 1300
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27174#file27174line1300>
> >
> >     Why not just if (res <= 0)

Changed


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/res/res_agi.c, line 1330
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27174#file27174line1330>
> >
> >     Same here

And here


> On April 11, 2012, 3:34 p.m., rmudgett wrote:
> > /branches/1.8/res/res_musiconhold.c, lines 645-646
> > <https://reviewboard.asterisk.org/r/1863/diff/1/?file=27175#file27175line645>
> >
> >     This pthread_testcancel() is now redundant.
> >

Removed


- Matt


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


On April 11, 2012, 2:26 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1863/
> -----------------------------------------------------------
> 
> (Updated April 11, 2012, 2:26 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Through a static analysis tool, a large number of errors were found that dealt with using potential negative return values in harmful ways.  The following is a summary of the changes for the various affected files:
> 
> * app_voicemail
>   - a negative result from lseek is later passed into mmap as the size of the memory map
>   - a negative result from read closes the various file descriptors and unlinks the output file, but the next check against the return value only checks to see if its non-zero
> * chan_agent - ast_channel_unlock returns a negative value if the item passed to it is NULL or an invalid ao2 object.  strerror cannot accept a negative value.
> * chan_dahdi - if dahdi_get_index returns a negative value, we index directly into an array
> * format_* - various errors not checking return values of ftello and fseek, and passing the results into functions that expect non-negative values
> * func_env - various places where the result of ftello was directly used as input to functions that expect non-negative values
> * asterisk
>   - the request to read returns a negative value.  This causes us to enter into the retry logic.  If we attempt to reconnect and succeed, we would normally proceed in the for(;;) loop and attempt to index into buf using the negative return value from read.  Instead, if we do reconnect, we immediately return to the beginning of the for(;;) loop and attempt a new read.
>   - similar situation - if a read fails, don't attempt to index into a buffer using the return value
> * frame - if we can't determine a preferred codec using the provided values, don't attempt to use an index value that never got set
> * manager 
>   - various failures of mkstemp, lseek were not checked for and could be provided to methods that don't handle negative numbers
>   - passing a negative result from lseek into mmap as the size of the memory map
> * translate - powerof can return a negative result if no bits are set, which would then be used as an index into an array
> * res_agi - if read returns an error, we treated it as if bytes were read from the pipe
> * res_musiconhold - if we fail to spawn spawn_mp3 returns a negative number, we wait a bit and attempt again later.  However, the return 'file descriptor' srcfd is later passed into read.
> * res_rtp_asterisk - ast_rtp_codecs_payload_code can return a negative value if no codecs are found that match between instance1 and the specified payload.  If that's the case, the bridge should be broken, as there are no compatible formats between the two different endpoints (and we shouldn't index into an array using the return value)
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/translate.c 361945 
>   /branches/1.8/main/manager.c 361945 
>   /branches/1.8/main/frame.c 361945 
>   /branches/1.8/main/asterisk.c 361945 
>   /branches/1.8/funcs/func_env.c 361945 
>   /branches/1.8/formats/format_wav_gsm.c 361945 
>   /branches/1.8/formats/format_wav.c 361945 
>   /branches/1.8/formats/format_vox.c 361945 
>   /branches/1.8/formats/format_sln16.c 361945 
>   /branches/1.8/formats/format_sln.c 361945 
>   /branches/1.8/formats/format_siren7.c 361945 
>   /branches/1.8/formats/format_siren14.c 361945 
>   /branches/1.8/formats/format_pcm.c 361945 
>   /branches/1.8/formats/format_h263.c 361945 
>   /branches/1.8/formats/format_h264.c 361945 
>   /branches/1.8/formats/format_ilbc.c 361945 
>   /branches/1.8/formats/format_g729.c 361945 
>   /branches/1.8/formats/format_gsm.c 361945 
>   /branches/1.8/formats/format_g719.c 361945 
>   /branches/1.8/formats/format_g723.c 361945 
>   /branches/1.8/channels/chan_agent.c 361945 
>   /branches/1.8/channels/chan_dahdi.c 361945 
>   /branches/1.8/apps/app_voicemail.c 361945 
>   /branches/1.8/res/res_agi.c 361945 
>   /branches/1.8/res/res_musiconhold.c 361945 
>   /branches/1.8/res/res_rtp_asterisk.c 361945 
> 
> Diff: https://reviewboard.asterisk.org/r/1863/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt
> 
>

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

More information about the asterisk-dev mailing list