[FFmpeg-devel] [PATCH] avcodec/decode: Clear format on ff_get_buffer() failure

[Anton Khirnov]

Quoting Michael Niedermayer (2020-03-24 21:23:58)
> On Tue, Mar 24, 2020 at 10:59:21AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2020-03-24 01:41:43)
> > > Fixes: out of array access
> > > Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/decode.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > > index af6bb3f952..fb22ef0ef3 100644
> > > --- a/libavcodec/decode.c
> > > +++ b/libavcodec/decode.c
> > > @@ -1972,6 +1972,7 @@ int ff_get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> > >      if (ret < 0) {
> > >          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > >          frame->width = frame->height = 0;
> > > +        frame->format = -1;
> > 
> > This looks ad-hoc. And also unnecessary, since get_buffer_internal() is
> > already calling av_frame_unref() on failure.
> > Seems it's just missing the cleanup goto on ff_decode_frame_props()
> > failure.
> 
> will post a patch to fix that cleanup but i have to say that i 
> feel a bit uneasy that a fix to out of array access is "not forgetting
> cleanup on any path"

As I see it, we should not be fixing a segfault/invalid access. That
segfault is not in itself a bug, merely a symptom of one. The actual bug
is that the program got into an invalid state, which we fix by making
the code fail in a safe way.

I believe that approach is better than sprinkling random checks in
various places that only detect some limited instances of invalid state,
but do not prevent it from happening in the first place.

> Ill add a assert so if another path leads to this, it at least gets
> stoped/clearly caught.

I think it might be clearest to merge ff_get_buffer() and
get_buffer_internal(). Since the purpose of separation is just to print
an error message on failure and get_buffer_internal() already contains a
cleanup block, we can just make all failed paths go through cleanup.
That will make it obvious that the frame is always clean on failure and
prevent not just this crash but any other crashes possibly arising from
the frame not being clean.

-- 
Anton Khirnov