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

[Stefano Sabatini]

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

2) Introduce some generic function in libavutil such as av_safe_mul32()
   as proposed in the previous post and use it.

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.

Regards.
-- 
FFmpeg = Frightening Free Moronic Pitiless Eager Geisha