diff mbox series

[FFmpeg-devel,1/4] avcodec/svq3: dont crash on free_picture(NULL)

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)
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Sept. 24, 2020, 8:20 p.m. UTC
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(+)

Comments

Andreas Rheinhardt Sept. 24, 2020, 8:47 p.m. UTC | #1
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 Sept. 24, 2020, 8:51 p.m. UTC | #2
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 Sept. 26, 2020, 11:32 a.m. UTC | #3
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 mbox series

Patch

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]);
     }