[asterisk-dev] [Code Review]: Don't read past then end of our int when calling write() in __ast_queue_frame

[Terry Wilson]

> On Nov. 12, 2011, 12:21 a.m., wdoekes wrote:
> > /branches/1.8/main/channel.c, lines 1502-1505
> > <https://reviewboard.asterisk.org/r/1583/diff/1/?file=21722#file21722line1502>
> >
> >     Consider renaming 'blah' to something. Even 'temp' would be better, but in this case perhaps 'onoff' or 'nonzero'.
> >     
> >     int buf[variable] is not C90 according to gcc (compile with -pedantic). It will probably work with all gcc's, but other compilers may complain.
> >     
> >     Also, I haven't seen this construct anywhere else in the source. Maybe use alloca instead?

I had it in my mind as invalid under any circumstances and asked Kevin about it when I saw it. Apparently I'm just *really* old fashioned. It is valid C99, and GCC has supported it since C89 or 90. If we can't start using a feature that has been around in GCC for over 20 years and has been standardized for over 10, then something has gone wrong somewhere. Also, from the README in the source tree:

   Asterisk requires either the GNU Compiler Collection (GCC) version
   3.0 or higher, or a compiler that supports the C99 specification and some of
   the gcc language extensions.

We use the same variable name here and in __ast_read(). I'll change the name in a separate patch when I change it to writing a char instead of an int.


- Terry


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


On Nov. 11, 2011, 10:25 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1583/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2011, 10:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> int blah = 1;
> ...
> write(chan->alertpipe[1], &blah, new_frames * sizeof(blah)) != (new_frames * sizeof(blah)))
> 
> is only valid when new_frames == 1. Otherwise we start reading into adjacent variables declared on the stack. The read end discards what is read, so the values don't matter but it's not a good idea to read past where we want event though new_frames is almost always 1 and should never be large. This patch is basically taken out of kpfleming's eventfd branch, as he mentioned that he remembered fixing it there when I talked to him about this issue.
> 
> Another thing that could be done is to change blah to a char[] here and where we read from the alertpipe. There is no reason to write four bytes when we could just write one, but I can address that in a later patch.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/channel.c 344438 
> 
> Diff: https://reviewboard.asterisk.org/r/1583/diff
> 
> 
> Testing
> -------
> 
> It compiles. Calls still seem to work.
> 
> 
> Thanks,
> 
> Terry
> 
>

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