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

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

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