[FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Vittorio Giovara]

On Thu, Jan 17, 2019 at 6:34 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Wed, Jan 16, 2019 at 09:05:18PM -0500, Vittorio Giovara wrote:
> > On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer
> <michael at niedermayer.cc>
> > wrote:
> >
> > > Fixes: Timeout
> > > Fixes:
> > >
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > >
> > > Before:
> > >
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > > in 15423 ms
> > > After:
> > >
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > > in 190 ms
> > >
> > > 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/rscc.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> > > index 7921f149ed..fa066afd7f 100644
> > > --- a/libavcodec/rscc.c
> > > +++ b/libavcodec/rscc.c
> > > @@ -64,6 +64,7 @@ typedef struct RsccContext {
> > >      /* zlib interaction */
> > >      uint8_t *inflated_buf;
> > >      uLongf inflated_size;
> > > +    int valid_pixels
> > >  } RsccContext;
> > >
> > >  static av_cold int rscc_init(AVCodecContext *avctx)
> > > @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext
> *avctx,
> > > void *data,
> > >          memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
> > >      }
> > >
> > > -    *got_frame = 1;
> > > +    // We only return a picture when more than 5% is undameged, this
> > > avoids copying nearly broken frames around
> > > +    if (ctx->valid_pixels < ctx->inflated_size)
> > > +        ctx->valid_pixels += pixel_size;
> > > +    if (ctx->valid_pixels >= ctx->inflated_size/20)
> > >
> >
> > this feels arbitrary and hackish, for very little gain, IMO
>
> Would you be ok with rejecting RSCC files without a keyframe ?
> or more precissely all frames before a keyframe and thus if there is
> no keyframe the whole file
> (that would be a superset of what this patch rejects)
>

If that could be achieved in a clean way without codec-specific additions,
sure why not. It could maybe exploit the AV_FRAME_FLAG_CORRUPT flag and
some other combination of codec flag to expose this feature. However I'm
still adamant at "fixing" issues in the code found by external code tools
in this way: at most it is the fuzzer that should be better instrumented to
accept longer or no timeouts for this codec.
-- 
Vittorio