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

[Andreas Rheinhardt]

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.

>>       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.