[FFmpeg-devel] [libav-devel] [PATCH] vp8: check for too large dimensions

[Ronald S. Bultje]

Hi,

On Sun, Jun 7, 2015 at 12:44 PM, Andreas Cadhalpun <
andreas.cadhalpun at googlemail.com> wrote:

> Hi Ronald,
>
> On 07.06.2015 17:52, Ronald S. Bultje wrote:
> > On Sun, Jun 7, 2015 at 10:05 AM, Andreas Cadhalpun <
> > andreas.cadhalpun at googlemail.com> wrote:
> >
> >> +#define MARGIN (16 << 2)
> >> +#define MAX_MB_SIZE (((INT16_MAX - MARGIN) >> 6) + 1)
> >>
> >
> > So this is roughly 9 bits.
> >
> >
> >> +    if (s->avctx->coded_width  > MAX_MB_SIZE * 16 ||
> >> +        s->avctx->coded_height > MAX_MB_SIZE * 16) {
> >> +            av_log(s->avctx, AV_LOG_ERROR, "too large dimensions
> %dx%d\n",
> >> +                   s->avctx->coded_width, s->avctx->coded_height);
> >> +            return AVERROR_INVALIDDATA;
> >> +        }
> >
> >
> > And this makes it 13, so we have a max w/h of around ~8k.
>
> That is 8176 and thus mb_width/mb_height can maximally be 511.
>
> > That's not very
> > big. Does that conform to any code in libvpx? Is it possible mv_max.x/y
> > should be 32bit instead?
>
> The type of mv_max is VP56mv, which is used in quite many places, so
> I'm not overly confident in changing that.
> And it seems libvpx uses short [1], which should be equivalent to int16_t.
>
> > We can't simply claim that 8k is not a valid dimension, that would make
> us
> > a vp8-incompatible decoder if vp8 stream dimensions are allowed to be
> >8k.
> > This could particularly happen in images (webp).
>
> I'd change the error to AVERROR_PATCHWELCOME as Michael suggested, so feel
> free to fix this limitation. ;)


No, no, not so easy. For example, keyframes (like webp) would decode just
fine even thougb mvmin/max is broken, so we're causing a regression here of
something that worked fine before.

Is this an actual bug? What is the behaviour that you're trying to fix? Is
this some overflow noticed on some generated/fuzzed bitstream with
-fsanitize=integer? Or are we decoding a sample differently from libvpx? Or
something else?

Ronald