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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081228/76905c79/attachment.pgp>