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

[Michael Niedermayer]

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
> 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"


[...]
>  
> > > 2) Introduce some generic function in libavutil such as av_safe_mul32()
> > >    as proposed in the previous post and use it.
> > 
> > if you want to check for an overflow, check for it:
> > 
> > if(a*(uint64_t)b > INT_MAX)
> >     error
> > something(a*b);
> > 
> > 
> > using this silly av_safe_mul32() will only make the code more messy:
> > 
> > int32_t tmp;
> > if(av_safe_mul32(&tmp, a, b) < 0)
> >     error;
> > something(tmp);
> 
> Yes, though for more than two operands it still could be useful.

like for (a*b+c)*d
?


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20081227/9e2d8e74/attachment.pgp>