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

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

[Kacper Michajłow]

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);
-- 
2.45.1

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

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

[Jan Ekström]

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

Re: [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@gmail.com> wrote:
>
> 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.

Applied to master as 4ba9ae7742a6f8a29d6486e25ff5709a075edb5b .

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