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

[James Almer]

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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>