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

[Jan Ekström]

On Wed, Jan 29, 2025 at 10:21 PM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Sun, Jan 26, 2025 at 9:41 PM Kacper Michajłow <kasper93 at gmail.com> wrote:
> >
> > Signed-off-by: Kacper Michajłow <kasper93 at 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.

Applied to master as 4ba9ae7742a6f8a29d6486e25ff5709a075edb5b .

Jan