| Message ID | 20200924202039.30285-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/svq3: dont crash on free_picture(NULL) | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Michael Niedermayer: > 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: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/svq3.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c > index fb7b992496..2da757bee9 100644 > --- a/libavcodec/svq3.c > +++ b/libavcodec/svq3.c > @@ -1323,6 +1323,10 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx) > static void free_picture(AVCodecContext *avctx, SVQ3Frame *pic) > { > int i; > + > + if (!pic) > + return; > + > for (i = 0; i < 2; i++) { > av_freep(&pic->motion_val_buf[i]); > } > This surprises me a lot: Since 96061c5a4f690c3ab49e4458701bb013fd3dd57f, the pointers to the pictures are set to something different from NULL directly at the beginning of the init function; they are never set to NULL afterwards, they are only swapped with pointers to other pictures. So how can they ever be NULL? Has the init function not been properly called? (And if any of these pictures were NULL, then svq3_decode_end() will call av_frame_free(NULL), which is unsafe, but doesn't crash currently; the situation would change if the AVFrame * were somewhere else than the beginning of SVQ3Frame.) - Andreas
Andreas Rheinhardt: > Michael Niedermayer: >> 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: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/svq3.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c >> index fb7b992496..2da757bee9 100644 >> --- a/libavcodec/svq3.c >> +++ b/libavcodec/svq3.c >> @@ -1323,6 +1323,10 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx) >> static void free_picture(AVCodecContext *avctx, SVQ3Frame *pic) >> { >> int i; >> + >> + if (!pic) >> + return; >> + >> for (i = 0; i < 2; i++) { >> av_freep(&pic->motion_val_buf[i]); >> } >> > This surprises me a lot: Since 96061c5a4f690c3ab49e4458701bb013fd3dd57f, > the pointers to the pictures are set to something different from NULL > directly at the beginning of the init function; they are never set to > NULL afterwards, they are only swapped with pointers to other pictures. > So how can they ever be NULL? Has the init function not been properly > called? (And if any of these pictures were NULL, then svq3_decode_end() > will call av_frame_free(NULL), which is unsafe, but doesn't crash > currently; the situation would change if the AVFrame * were somewhere > else than the beginning of SVQ3Frame.) > > - Andreas > I just noticed that avcodec_open2() will call the codec's close function even when init has never been called when the FF_CODEC_CAP_INIT_CLEANUP flag is set. This is against the documentation of said flag and it is also complete nonsense. Will send a patch for this. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Michael Niedermayer: >>> 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: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/svq3.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c >>> index fb7b992496..2da757bee9 100644 >>> --- a/libavcodec/svq3.c >>> +++ b/libavcodec/svq3.c >>> @@ -1323,6 +1323,10 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx) >>> static void free_picture(AVCodecContext *avctx, SVQ3Frame *pic) >>> { >>> int i; >>> + >>> + if (!pic) >>> + return; >>> + >>> for (i = 0; i < 2; i++) { >>> av_freep(&pic->motion_val_buf[i]); >>> } >>> >> This surprises me a lot: Since 96061c5a4f690c3ab49e4458701bb013fd3dd57f, >> the pointers to the pictures are set to something different from NULL >> directly at the beginning of the init function; they are never set to >> NULL afterwards, they are only swapped with pointers to other pictures. >> So how can they ever be NULL? Has the init function not been properly >> called? (And if any of these pictures were NULL, then svq3_decode_end() >> will call av_frame_free(NULL), which is unsafe, but doesn't crash >> currently; the situation would change if the AVFrame * were somewhere >> else than the beginning of SVQ3Frame.) >> >> - Andreas >> > I just noticed that avcodec_open2() will call the codec's close function > even when init has never been called when the FF_CODEC_CAP_INIT_CLEANUP > flag is set. This is against the documentation of said flag and it is > also complete nonsense. Will send a patch for this. > Done: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-September/270336.html - Andreas
diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c index fb7b992496..2da757bee9 100644 --- a/libavcodec/svq3.c +++ b/libavcodec/svq3.c @@ -1323,6 +1323,10 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx) static void free_picture(AVCodecContext *avctx, SVQ3Frame *pic) { int i; + + if (!pic) + return; + for (i = 0; i < 2; i++) { av_freep(&pic->motion_val_buf[i]); }
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: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/svq3.c | 4 ++++ 1 file changed, 4 insertions(+)