[FFmpeg-devel,2/2] avcodec/vp56: Require not any undamaged frame for concealment but one of comparable size

Fixes: timeout in 758/clusterfuzz-testcase-4720832028868608

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vp56.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi,

On Wed, Mar 8, 2017 at 10:07 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> Fixes: timeout in 758/clusterfuzz-testcase-4720832028868608
>
> Found-by: continuous fuzzing process https://github.com/google/oss-
> fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vp56.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index 0010408847..bccb424903 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -710,7 +710,7 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx,
> void *data,
>                  int ret = vp56_decode_mb(s, mb_row, mb_col, is_alpha);
>                  if (ret < 0) {
>                      damaged = 1;
> -                    if (!s->have_undamaged_frame) {
> +                    if (s->have_undamaged_frame < s->mb_width *
> s->mb_height) {
>                          s->discard_frame = 1;
>                          return AVERROR_INVALIDDATA;
>                      }
> @@ -732,7 +732,7 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx,
> void *data,
>      }
>
>      if (!damaged)
> -        s->have_undamaged_frame = 1;
> +        s->have_undamaged_frame = s->mb_width * s->mb_height;


You know very well that this makes the memory issue go away but isn't doing
the right thing if width1!=width2 && height1!=height2 but width1*height1 ==
 width2*height2. This is obviously because vpN codecs up to and including
vp8 don't include scalable MC.

Can you do this right and only allow this if frame/ref width and height
both match, not just their product?

Ronald

On Thu, Mar 09, 2017 at 07:59:37AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Mar 8, 2017 at 10:07 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > Fixes: timeout in 758/clusterfuzz-testcase-4720832028868608
> >
> > Found-by: continuous fuzzing process https://github.com/google/oss-
> > fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/vp56.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> > index 0010408847..bccb424903 100644
> > --- a/libavcodec/vp56.c
> > +++ b/libavcodec/vp56.c
> > @@ -710,7 +710,7 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx,
> > void *data,
> >                  int ret = vp56_decode_mb(s, mb_row, mb_col, is_alpha);
> >                  if (ret < 0) {
> >                      damaged = 1;
> > -                    if (!s->have_undamaged_frame) {
> > +                    if (s->have_undamaged_frame < s->mb_width *
> > s->mb_height) {
> >                          s->discard_frame = 1;
> >                          return AVERROR_INVALIDDATA;
> >                      }
> > @@ -732,7 +732,7 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx,
> > void *data,
> >      }
> >
> >      if (!damaged)
> > -        s->have_undamaged_frame = 1;
> > +        s->have_undamaged_frame = s->mb_width * s->mb_height;
> 
> 
> You know very well that this makes the memory issue go away but isn't doing
> the right thing if width1!=width2 && height1!=height2 but width1*height1 ==
>  width2*height2. This is obviously because vpN codecs up to and including
> vp8 don't include scalable MC.
> 
> Can you do this right and only allow this if frame/ref width and height
> both match, not just their product?

you assume that there is a out of array access and this is fixing it
and argue that this is the wrong fix for it.

You are correct that this would be the wrong fix if that was the bug.

Its possible there is such a bug, but i have not seen that.
What this is about is a timeout, as described in the commit message

a small file with a tiny initial
frame followed by frames with huge resolution but very few bits
the code is timing out as the error concealment takes alot of time
for high resolutions, no memory anomalies occured here when the
concealment code runs so any reference frame must be large enough.

The solution this patch implements is to require at least a undamagd
frame with X MBs before allowing concealment of similarly high
resolution frames. This undamagd frame is quite possibly not used
by the concealment its rather a check to make sure the file is not
just "empty"
This directly combats the issue of crafted files that are very small
bytewise but take a huge amount of time to decode.
Its quite possible the fuzzer will find a hole in this and we may
require to count undamaged mbs over several frames to weed out
these slow to decode empty timeout cases, we will only know once
the easier case is closed

[...]