| Message ID | 20200926102804.228089-18-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 5bc74d06dad35d00b5925b1c76208aeaf40a2dbb |
| Headers | show |
| Series | [FFmpeg-devel,01/25] avcodec/photocd: Simplify parsing Huffman tables a bit | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sat, Sep 26, 2020 at 12:27:57PM +0200, Andreas Rheinhardt wrote: > avcodec_open2() also called the AVCodec's close function if an error > happened before init had ever been called if the AVCodec has the > FF_CODEC_CAP_INIT_CLEANUP flag set. This is against the documentation of > said flag: "The codec allows calling the close function for deallocation > even if the init function returned a failure." > > E.g. the SVQ3 decoder is not ready to be closed if init has never been > called. > > Fixes: NULL dereference > Fixes: 25762/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SVQ3_fuzzer-5716279070294016 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > If someone were to call avcodec_alloc_context3(NULL) and manually fill > in avctx->codec, then the close function might even be called before > avctx->priv_data has been allocated. > probably ok. please wait for others to also comment. > libavcodec/utils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index cf0a55f26d..94b9e94584 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -931,6 +931,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > || avci->frame_thread_encoder)) { > ret = avctx->codec->init(avctx); > if (ret < 0) { > + codec_init_ok = -1; > goto free_and_end; > } > codec_init_ok = 1; > @@ -1022,8 +1023,8 @@ end: > return ret; > free_and_end: > if (avctx->codec && avctx->codec->close && > - (codec_init_ok || > - (avctx->codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP))) > + (codec_init_ok > 0 || (codec_init_ok < 0 && > + avctx->codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP))) > avctx->codec->close(avctx); > > if (HAVE_THREADS && avci->thread_ctx) > -- > 2.25.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, Sep 26, 2020 at 12:27:57PM +0200, Andreas Rheinhardt wrote: > avcodec_open2() also called the AVCodec's close function if an error > happened before init had ever been called if the AVCodec has the > FF_CODEC_CAP_INIT_CLEANUP flag set. This is against the documentation of > said flag: "The codec allows calling the close function for deallocation > even if the init function returned a failure." > > E.g. the SVQ3 decoder is not ready to be closed if init has never been > called. > > Fixes: NULL dereference > Fixes: 25762/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SVQ3_fuzzer-5716279070294016 confirmed thx [...]
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index cf0a55f26d..94b9e94584 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -931,6 +931,7 @@ FF_ENABLE_DEPRECATION_WARNINGS || avci->frame_thread_encoder)) { ret = avctx->codec->init(avctx); if (ret < 0) { + codec_init_ok = -1; goto free_and_end; } codec_init_ok = 1; @@ -1022,8 +1023,8 @@ end: return ret; free_and_end: if (avctx->codec && avctx->codec->close && - (codec_init_ok || - (avctx->codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP))) + (codec_init_ok > 0 || (codec_init_ok < 0 && + avctx->codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP))) avctx->codec->close(avctx); if (HAVE_THREADS && avci->thread_ctx)
avcodec_open2() also called the AVCodec's close function if an error happened before init had ever been called if the AVCodec has the FF_CODEC_CAP_INIT_CLEANUP flag set. This is against the documentation of said flag: "The codec allows calling the close function for deallocation even if the init function returned a failure." E.g. the SVQ3 decoder is not ready to be closed if init has never been called. Fixes: NULL dereference Fixes: 25762/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SVQ3_fuzzer-5716279070294016 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- If someone were to call avcodec_alloc_context3(NULL) and manually fill in avctx->codec, then the close function might even be called before avctx->priv_data has been allocated. libavcodec/utils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)