[FFmpeg-devel] Overflow check for frame_size in v4l.c

[Stefano Sabatini]

On date Sunday 2008-12-28 02:08:19 +0100, Michael Niedermayer encoded:
> On Sun, Dec 28, 2008 at 01:25:36AM +0100, Stefano Sabatini wrote:
> > On date Saturday 2008-12-27 23:33:19 +0100, Michael Niedermayer encoded:
> > > On Sat, Dec 27, 2008 at 06:01:37PM +0100, Stefano Sabatini wrote:
> > > > On date Saturday 2008-12-27 17:16:05 +0100, Michael Niedermayer encoded:
> > > > > On Sat, Dec 27, 2008 at 03:12:59PM +0100, Stefano Sabatini wrote:
> > > > > > On date Saturday 2008-12-27 14:36:37 +0100, Michael Niedermayer encoded:
> > > > [...]
> > > > > > > > > I think the check is insufficient and more not less checking is needed
> > > > > > > > > 
> > > > > > > > >  frame_size = s->video_win.width * s->video_win.height * video_formats[j].depth / 8;
> > > > > > > > > 
> > > > > > > > > will not work with 32767*32767*...
> > > > > > > > 
> > > > > > > > OK, 32767 = 2^15 -1.
> > > > > > > > 
> > > > > > > > We may then check for 16383 = 2^14 -1 (check the patch below), or
> > > > > > > > maybe some function like these ones may help:
> > > > > > > 
> > > > > > > avcodec_check_dimensions()
> > > > > > 
> > > > > > I don't think it is a good idea to use that function here, its domain
> > > > > 
> > > > > its exactly what the function is designed to check.
> > > > 
> > > > /**
> > > >  * Checks if the given dimension of a picture is valid, meaning that all
> > > >  * bytes of the picture can be addressed with a signed int.
> > > >  *
> > > >  * @param[in] w Width of the picture.
> > > >  * @param[in] h Height of the picture.
> > > >  * @return Zero if valid, a negative value if invalid.
> > > >  */
> > > > int avcodec_check_dimensions(void *av_log_ctx, unsigned int w, unsigned int h);
> > > > 
> > > > The definition is promising, yet I'm missing the part which deals with
> > > > the pixel depth.
> > > > 
> > > > Then reading the code my confusion increased, as I can't figure out
> > > > the meaning of the code (what I -- naively -- expected was something like:
> > > > w * h < (INT_MAX -1 ) / MAX_DEPTH) (I guess those 128s are for
> > 
> > uh, I meant w * h < INT_MAX / MAX_DEPTH
> > 
> > > > alignment reasons?):
> > > 
> > > some codecs work with fixed sized blocks like the 16x16 macroblocks ...
> > > i guess the 128 and 4 should be replaced by "named constants"
> > 
> > My only problem with the use of this function is that it forces checks
> > stricter than the necessary on most occasions, indeed not always depth
> > is 4 and not only the fixed sized block considerations applied.
> > 
> > Anyway new try attached.
> > 
> > Note that in the v4l case we need to move the check *after* the size
> > is eventually automatically set (furthermore if w/h is zero then
> > avcodec_check_dimensions() will fail).
> 
> looks ok

Applied, I'll work on some patches for v4l2.c as long as I'll be able to
test it (now I'm using gspca 1, linux 2.6.28 contains gspca 2 with
support for v4l2 but isn't yet included in Debian and I'm too lazy to
compile my own kernel or try out the v4l sources).

Regards.
-- 
FFmpeg = Faboulous and Fundamental Mournful Philosofic Everlasting Gymnast