diff mbox series

[FFmpeg-devel,1/3] avcodec/av1dec: Fix leak in case of failure

Message ID 20201204225740.1506052-1-andreas.rheinhardt@gmail.com
State Accepted
Commit e546d029198950f589f7d9820970e599fba2ad30
Headers show
Series [FFmpeg-devel,1/3] avcodec/av1dec: Fix leak in case of failure
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Dec. 4, 2020, 10:57 p.m. UTC
A reference to an AV1RawFrameHeader and consequently the
AV1RawFrameHeader itself and everything it has a reference to leak
if the hardware has no AV1 decoding capabilities. It happens e.g.
in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked
because the return value of ffmpeg (which indicates failure when using
Valgrind or ASAN) is ignored when doing tests of type md5.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
I switched the av_buffer_ref() and update_context_with_frame_header()
because the latter does not need any cleanup on failure.

Also notice that actual decoding with this patch applied is completely
untested.

 libavcodec/av1dec.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

James Almer Dec. 4, 2020, 11:13 p.m. UTC | #1
On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote:
> A reference to an AV1RawFrameHeader and consequently the
> AV1RawFrameHeader itself and everything it has a reference to leak
> if the hardware has no AV1 decoding capabilities.

It looks like it can happen if get_buffer() fails even with hardware 
support.

> It happens e.g.
> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked
> because the return value of ffmpeg (which indicates failure when using
> Valgrind or ASAN) is ignored when doing tests of type md5.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> I switched the av_buffer_ref() and update_context_with_frame_header()
> because the latter does not need any cleanup on failure.
> 
> Also notice that actual decoding with this patch applied is completely
> untested.
> 
>   libavcodec/av1dec.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index 1589b8f0c0..d7b2ac9d46 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
>       AVFrame *frame;
>       int ret;
>   
> -    f->header_ref = av_buffer_ref(s->header_ref);
> -    if (!f->header_ref)
> -        return AVERROR(ENOMEM);
> -
> -    f->raw_frame_header = s->raw_frame_header;
> -
>       ret = update_context_with_frame_header(avctx, header);
>       if (ret < 0) {
>           av_log(avctx, AV_LOG_ERROR, "Failed to update context with frame header\n");
>           return ret;
>       }
>   
> +    f->header_ref = av_buffer_ref(s->header_ref);
> +    if (!f->header_ref)
> +        return AVERROR(ENOMEM);
> +
> +    f->raw_frame_header = s->raw_frame_header;
> +
>       if ((ret = ff_thread_get_buffer(avctx, &f->tf, AV_GET_BUFFER_FLAG_REF)) < 0)
> -        return ret;
> +        goto fail;
>   
>       frame = f->tf.f;
>       frame->key_frame = header->frame_type == AV1_FRAME_KEY;
> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
>           if (hwaccel->frame_priv_data_size) {
>               f->hwaccel_priv_buf =
>                   av_buffer_allocz(hwaccel->frame_priv_data_size);
> -            if (!f->hwaccel_priv_buf)
> +            if (!f->hwaccel_priv_buf) {
> +                ret = AVERROR(ENOMEM);
>                   goto fail;
> +            }
>               f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
>           }
>       }
> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
>   
>   fail:

Just to be safe, add a ret = 0 above this line.

>       av1_frame_unref(avctx, f);
> -    return AVERROR(ENOMEM);
> +    return ret;
>   }
>   
>   static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,

LGTM, and while unrelated to this fix, this also reveals that in some 
scenarios, decoding without hardware support reaches this point, when 
it's meant to abort when parsing the sequence header and being unable to 
set up a hardware pixel format in avctx.

Looks like when parsing a second sequence header (Like in the second 
keyframe) where s->pix_fmt is already set to a software format, 
get_pixel_format() is not called, and so decoding proceeds to deal with 
frames.
Andreas Rheinhardt Dec. 4, 2020, 11:19 p.m. UTC | #2
James Almer:
> On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote:
>> A reference to an AV1RawFrameHeader and consequently the
>> AV1RawFrameHeader itself and everything it has a reference to leak
>> if the hardware has no AV1 decoding capabilities.
> 
> It looks like it can happen if get_buffer() fails even with hardware
> support.
> 
>> It happens e.g.
>> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked
>> because the return value of ffmpeg (which indicates failure when using
>> Valgrind or ASAN) is ignored when doing tests of type md5.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> I switched the av_buffer_ref() and update_context_with_frame_header()
>> because the latter does not need any cleanup on failure.
>>
>> Also notice that actual decoding with this patch applied is completely
>> untested.
>>
>>   libavcodec/av1dec.c | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>> index 1589b8f0c0..d7b2ac9d46 100644
>> --- a/libavcodec/av1dec.c
>> +++ b/libavcodec/av1dec.c
>> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext
>> *avctx, AV1Frame *f)
>>       AVFrame *frame;
>>       int ret;
>>   -    f->header_ref = av_buffer_ref(s->header_ref);
>> -    if (!f->header_ref)
>> -        return AVERROR(ENOMEM);
>> -
>> -    f->raw_frame_header = s->raw_frame_header;
>> -
>>       ret = update_context_with_frame_header(avctx, header);
>>       if (ret < 0) {
>>           av_log(avctx, AV_LOG_ERROR, "Failed to update context with
>> frame header\n");
>>           return ret;
>>       }
>>   +    f->header_ref = av_buffer_ref(s->header_ref);
>> +    if (!f->header_ref)
>> +        return AVERROR(ENOMEM);
>> +
>> +    f->raw_frame_header = s->raw_frame_header;
>> +
>>       if ((ret = ff_thread_get_buffer(avctx, &f->tf,
>> AV_GET_BUFFER_FLAG_REF)) < 0)
>> -        return ret;
>> +        goto fail;
>>         frame = f->tf.f;
>>       frame->key_frame = header->frame_type == AV1_FRAME_KEY;
>> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>> AV1Frame *f)
>>           if (hwaccel->frame_priv_data_size) {
>>               f->hwaccel_priv_buf =
>>                   av_buffer_allocz(hwaccel->frame_priv_data_size);
>> -            if (!f->hwaccel_priv_buf)
>> +            if (!f->hwaccel_priv_buf) {
>> +                ret = AVERROR(ENOMEM);
>>                   goto fail;
>> +            }
>>               f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
>>           }
>>       }
>> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>> AV1Frame *f)
>>     fail:
> 
> Just to be safe, add a ret = 0 above this line.
> 
There is a "return 0;" above this line (the non-error path does not
include this av1_frame_unref()), so this makes no sense.

>>       av1_frame_unref(avctx, f);
>> -    return AVERROR(ENOMEM);
>> +    return ret;
>>   }
>>     static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,
> 
> LGTM, and while unrelated to this fix, this also reveals that in some
> scenarios, decoding without hardware support reaches this point, when
> it's meant to abort when parsing the sequence header and being unable to
> set up a hardware pixel format in avctx.
> 
Yeah, I get a screen full of error messages from this decoder.

> Looks like when parsing a second sequence header (Like in the second
> keyframe) where s->pix_fmt is already set to a software format,
> get_pixel_format() is not called, and so decoding proceeds to deal with
> frames.
James Almer Dec. 4, 2020, 11:25 p.m. UTC | #3
On 12/4/2020 8:19 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote:
>>> A reference to an AV1RawFrameHeader and consequently the
>>> AV1RawFrameHeader itself and everything it has a reference to leak
>>> if the hardware has no AV1 decoding capabilities.
>>
>> It looks like it can happen if get_buffer() fails even with hardware
>> support.
>>
>>> It happens e.g.
>>> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked
>>> because the return value of ffmpeg (which indicates failure when using
>>> Valgrind or ASAN) is ignored when doing tests of type md5.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> I switched the av_buffer_ref() and update_context_with_frame_header()
>>> because the latter does not need any cleanup on failure.
>>>
>>> Also notice that actual decoding with this patch applied is completely
>>> untested.
>>>
>>>    libavcodec/av1dec.c | 20 +++++++++++---------
>>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>>> index 1589b8f0c0..d7b2ac9d46 100644
>>> --- a/libavcodec/av1dec.c
>>> +++ b/libavcodec/av1dec.c
>>> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext
>>> *avctx, AV1Frame *f)
>>>        AVFrame *frame;
>>>        int ret;
>>>    -    f->header_ref = av_buffer_ref(s->header_ref);
>>> -    if (!f->header_ref)
>>> -        return AVERROR(ENOMEM);
>>> -
>>> -    f->raw_frame_header = s->raw_frame_header;
>>> -
>>>        ret = update_context_with_frame_header(avctx, header);
>>>        if (ret < 0) {
>>>            av_log(avctx, AV_LOG_ERROR, "Failed to update context with
>>> frame header\n");
>>>            return ret;
>>>        }
>>>    +    f->header_ref = av_buffer_ref(s->header_ref);
>>> +    if (!f->header_ref)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    f->raw_frame_header = s->raw_frame_header;
>>> +
>>>        if ((ret = ff_thread_get_buffer(avctx, &f->tf,
>>> AV_GET_BUFFER_FLAG_REF)) < 0)
>>> -        return ret;
>>> +        goto fail;
>>>          frame = f->tf.f;
>>>        frame->key_frame = header->frame_type == AV1_FRAME_KEY;
>>> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>>> AV1Frame *f)
>>>            if (hwaccel->frame_priv_data_size) {
>>>                f->hwaccel_priv_buf =
>>>                    av_buffer_allocz(hwaccel->frame_priv_data_size);
>>> -            if (!f->hwaccel_priv_buf)
>>> +            if (!f->hwaccel_priv_buf) {
>>> +                ret = AVERROR(ENOMEM);
>>>                    goto fail;
>>> +            }
>>>                f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
>>>            }
>>>        }
>>> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>>> AV1Frame *f)
>>>      fail:
>>
>> Just to be safe, add a ret = 0 above this line.
>>
> There is a "return 0;" above this line (the non-error path does not
> include this av1_frame_unref()), so this makes no sense.

Ah true, missed it. For some reason i assumed it was written the same 
way as some bsfs where it falls through. Nevermind then.

> 
>>>        av1_frame_unref(avctx, f);
>>> -    return AVERROR(ENOMEM);
>>> +    return ret;
>>>    }
>>>      static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,
>>
>> LGTM, and while unrelated to this fix, this also reveals that in some
>> scenarios, decoding without hardware support reaches this point, when
>> it's meant to abort when parsing the sequence header and being unable to
>> set up a hardware pixel format in avctx.
>>
> Yeah, I get a screen full of error messages from this decoder.

You'll get errors no matter what, but the idea is that they should not 
be get_buffer() ones, since they are a lot noisier.

> 
>> Looks like when parsing a second sequence header (Like in the second
>> keyframe) where s->pix_fmt is already set to a software format,
>> get_pixel_format() is not called, and so decoding proceeds to deal with
>> frames.
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index 1589b8f0c0..d7b2ac9d46 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -674,20 +674,20 @@  static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
     AVFrame *frame;
     int ret;
 
-    f->header_ref = av_buffer_ref(s->header_ref);
-    if (!f->header_ref)
-        return AVERROR(ENOMEM);
-
-    f->raw_frame_header = s->raw_frame_header;
-
     ret = update_context_with_frame_header(avctx, header);
     if (ret < 0) {
         av_log(avctx, AV_LOG_ERROR, "Failed to update context with frame header\n");
         return ret;
     }
 
+    f->header_ref = av_buffer_ref(s->header_ref);
+    if (!f->header_ref)
+        return AVERROR(ENOMEM);
+
+    f->raw_frame_header = s->raw_frame_header;
+
     if ((ret = ff_thread_get_buffer(avctx, &f->tf, AV_GET_BUFFER_FLAG_REF)) < 0)
-        return ret;
+        goto fail;
 
     frame = f->tf.f;
     frame->key_frame = header->frame_type == AV1_FRAME_KEY;
@@ -710,8 +710,10 @@  static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
         if (hwaccel->frame_priv_data_size) {
             f->hwaccel_priv_buf =
                 av_buffer_allocz(hwaccel->frame_priv_data_size);
-            if (!f->hwaccel_priv_buf)
+            if (!f->hwaccel_priv_buf) {
+                ret = AVERROR(ENOMEM);
                 goto fail;
+            }
             f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
         }
     }
@@ -719,7 +721,7 @@  static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f)
 
 fail:
     av1_frame_unref(avctx, f);
-    return AVERROR(ENOMEM);
+    return ret;
 }
 
 static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,