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

[Ronald S. Bultje]

Hi,

On Sun, Jun 7, 2015 at 4:39 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Sun, Jun 07, 2015 at 04:27:42PM -0400, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Sun, Jun 7, 2015 at 2:35 PM, Andreas Cadhalpun <
> > andreas.cadhalpun at googlemail.com> wrote:
> >
> > > Hi Ronald,
> > >
> > > On 07.06.2015 19:37, Ronald S. Bultje wrote:
> > > > On Sun, Jun 7, 2015 at 12:44 PM, Andreas Cadhalpun <
> > > > andreas.cadhalpun at googlemail.com> wrote:
> > > >> On 07.06.2015 17:52, Ronald S. Bultje wrote:
> > > >>> 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.
> > >
> > > Hmm, so maybe the check could be moved to decode_mb_row_no_filter?
> > >
> > > > 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?
> > >
> > > The overflow of mv_max can cause it to be smaller than mv_min, which
> causes
> > > av_clip to abort for ASSERT_LEVEL >= 2.
> >
> >
> > So what happens when you change mv_max/min to be integers (instead of
> > int16_t) without further touching VP56mv, e.g. by making mv_max/min a
> > VP8intminmaxmv (a newly created type just for this purpose, using int
> > instead of int16_t)?
>
> would this work with
> clamp_mv() ?
> it limits based in mv_max/min but writes into a VP56mv, so if these
> out of 16bit limits can actually be reached then the output of
> the clip would not fit in the 16bit destination ...
> but maybe this doesnt occur, ive not deeply investigated


Neither have I, I just don't like the sledgehammer approach of disabling
all vp8 decoding of >8k framesizes for something semi-obscure like this. I
think your approach of clamping min/max to int16_t range sounds most
appropriate. I'd also suggest to see what libvpx does (if anything) to make
sure we decode affected content the same.

Ronald