[FFmpeg-devel] Overflow check for frame_size in v4l.c
Sat Dec 27 23:33:19 CET 2008
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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel