| 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 |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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.
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
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 --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; }
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(+)