Re: [FFmpeg-devel] [PATCH] avformat/vqf: fix memory leak in add_metadata()

["Jan Ekström" <jeebjp@gmail.com>]

On Sun, Jan 26, 2025 at 9:41 PM Kacper Michajłow <kasper93@gmail.com> wrote:
>
> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> ---
>  libavformat/vqf.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/vqf.c b/libavformat/vqf.c
> index 58b1546f53..fbe54739cd 100644
> --- a/libavformat/vqf.c
> +++ b/libavformat/vqf.c
> @@ -66,10 +66,10 @@ static int add_metadata(AVFormatContext *s, uint32_t tag,
>          return AVERROR(ENOMEM);
>
>      ret = avio_read(s->pb, buf, len);
> -    if (ret < 0)
> -        return ret;
> -    if (len != ret)
> -        return AVERROR_INVALIDDATA;
> +    if (ret < 0 || ret != len) {
> +        av_free(buf);
> +        return ret < 0 ? ret : AVERROR_INVALIDDATA;
> +    }
>      buf[len] = 0;
>      AV_WL32(key, tag);
>      return av_dict_set(&s->metadata, key, buf, AV_DICT_DONT_STRDUP_VAL);

LGTM, will apply.

In some other cases I would prefer `ret = AVERROR_INVALIDDATA; goto
error;` kind of handling where the freeing of `buf` is handled in a
separate block at the end where additional clean-up logic may be added
as required, but with the complexity of this function this is quite
fine like this. As for `av_freep` vs `av_free`, the `buf` variable
being a local pointer on the stack with the freeing directly going to
a return statement, given the scope of the function it feels OK that
the plain free function is utilized here.

Best regards,
Jan
_______________________________________________
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".