On Wed, Jan 29, 2025 at 10:21 PM Jan Ekström <jee...@gmail.com> wrote:
>
> On Sun, Jan 26, 2025 at 9:41 PM Kacper Michajłow <kaspe...@gmail.com> wrote:
> >
> > Signed-off-by: Kacper Michajłow <kaspe...@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-requ...@ffmpeg.org with subject "unsubscribe".
