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

[Michael Niedermayer]

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:
> > On Sat, Dec 27, 2008 at 12:51:33PM +0100, Stefano Sabatini wrote:
> > > On date Friday 2008-12-26 14:09:20 +0100, Michael Niedermayer encoded:
> > > > On Fri, Dec 26, 2008 at 11:36:36AM +0100, Stefano Sabatini wrote:
> > > > > On date Friday 2008-12-26 00:52:12 +0100, Michael Niedermayer encoded:
> > > > > > On Wed, Dec 24, 2008 at 03:37:05PM +0100, Stefano Sabatini wrote:
> > > > [...]
> > > > > > [...]
> > > > > > 
> > > > > > 
> > > > > > > Index: ffmpeg/libavdevice/v4l.c
> > > > > > > ===================================================================
> > > > > > > --- ffmpeg.orig/libavdevice/v4l.c	2008-12-21 23:45:13.000000000 +0100
> > > > > > > +++ ffmpeg/libavdevice/v4l.c	2008-12-24 13:34:57.000000000 +0100
> > > > > > > @@ -84,11 +84,6 @@
> > > > > > >      }
> > > > > > >      s->time_base = ap->time_base;
> > > > > > >  
> > > > > > > -    if((unsigned)ap->width > 32767 || (unsigned)ap->height > 32767) {
> > > > > > > -        av_log(s1, AV_LOG_ERROR, "Capture size is out of range: %dx%d\n",
> > > > > > > -            ap->width, ap->height);
> > > > > > > -        return -1;
> > > > > > > -    }
> > > > > > >      s->video_win.width = ap->width;
> > > > > > >      s->video_win.height = ap->height;
> > > > > > >  
> > > > > > ?
> > > > > 
> > > > > The idea is that this check is useless, since either the VIDIOCSWIN
> > > > > either the VIDIOCMCAPTURE iotctl will perform a check on the size (but
> > > > > I don't know where the 32767 value comes from).
> > > > 
> > > > 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.


> is very specific and here we have to check the result of a
> multiplication with *three* operands.
> 
> I propose three possible choices:
> 

> 1) Implement a stricter check.
> 
>    Since we have
>    frame_size =  s->video_win.width * s->video_win.height * video_formats[j].depth / 8;
> 
>    and video_formats[j].depth / 8 is at maximum 3, then we have:
> 
>    X * X * 3 < 2 ^ 31 -1;
> 
>    that is:
>    X * X < (2^31 - 1) / 3
> 
>    max_X = tail (sqrt ((2^31 - 1) / 3)) = 26754
> 
>    so we could check for width/heigth <= 26754
> 
>    Or if we want to be more prudent, we can replace 3 with 8, then we have
>    max_X = 16383.
> 

this would reject cases that could never cause a problem, though iam not
against it, its better than what we have currently.


> 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);


> 
> 3) Leave the check as it is, if we're lucky no one will ever have any
>    problem with the non-strict-enough check, since valid values for
>    height and width are unlikely to generate an overflow.

I would prefer if we dont deal with security holes that way.


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/17959f3c/attachment.pgp>