diff mbox series

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

Message ID 20200324004143.8457-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/decode: Clear format on ff_get_buffer() failure | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer March 24, 2020, 12:41 a.m. UTC
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@niedermayer.cc>
---
 libavcodec/decode.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Anton Khirnov March 24, 2020, 9:59 a.m. UTC | #1
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@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.
Michael Niedermayer March 24, 2020, 8:23 p.m. UTC | #2
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@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"
Ill add a assert so if another path leads to this, it at least gets
stoped/clearly caught.

Thanks
Anton Khirnov March 25, 2020, 6:49 a.m. UTC | #3
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@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.
diff mbox series

Patch

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;
     }
     return ret;
 }