[FFmpeg-devel] [PATCH 1/2] avcodec/vp9: Check in decode_tiles() if there is data remaining

[Ronald S. Bultje]

Hi,

On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
> <michael at niedermayer.cc>
> > wrote:
> >
> > > On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> > > > Hi,
> > > >
> > > > On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
> <michael at niedermayer.cc
> > > >
> > > > wrote:
> > > >
> > > > > Fixes: Timeout
> > > > > Fixes:
> > > > > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> > > VP9_fuzzer-5707345857347584
> > > > >
> > > > > Found-by: continuous fuzzing process
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by
> > > > > <https://github.com/google/oss-fuzz/tree/master/projects/
> > > ffmpegSigned-off-by>:
> > > > > Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/vp9.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > > > index b1178c9c0c..4ca51ec108 100644
> > > > > --- a/libavcodec/vp9.c
> > > > > +++ b/libavcodec/vp9.c
> > > > > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
> *avctx,
> > > > >                          memset(lflvl_ptr->mask, 0,
> > > > > sizeof(lflvl_ptr->mask));
> > > > >                      }
> > > > >
> > > > > +                    if (td->c->end <= td->c->buffer &&
> td->c->bits >=
> > > 0) {
> > > > > +                        return AVERROR_INVALIDDATA;
> > > > > +                    }
> > > > >                      if (s->pass == 2) {
> > > > >                          decode_sb_mem(td, row, col, lflvl_ptr,
> > > > >                                        yoff2, uvoff2, BL_64X64);
> > > > >
> > > >
> > > > I don't think this matches spec. Implementations could use this to
> store
> > > > auxiliary data.
> > >
> > > This checks, or rather is intended to check for a premature end of the
> > > input.
> > > Am i missing something? because a premature end of input seems not to
> lead
> > > to more (auxiliary or other) data in the input.
> >
> >
> > Hm, I misread it, sorry about that, my bad. Please ignore my first
> review.
> >
>
> > Is end == buffer && bits == 0 something that can happen on valid input if
> > things just align properly? Otherwise looks OK.
>
> The same condition is used in vp5/6/8.
> I think this condition only occurs if the input ends. The part i do not
> know
> is if such premature ends might be a "valid compression"
>
> Either way, if this check misbehaves for a valid file then it should be
> reverted/removed unless its improv-able and improved.


<sarcasm> Yes, that's how production grade software works. </sarcasm> Can
you just make it not error out on the end == buffer && bits == 0 condition?
Or does that somehow not fix your timeout?

(Vp5/6 aren't used anywhere so nobody cares. Ffvp9 is used by e.g. Firefox
for Youtube playback so if it breaks 0.01% of files, we're going to
seriously screw a massive number of users.)

Ronald