[FFmpeg-devel] [PATCH] avformat/avidec: Fix memleak when error happens after creating DV stream
Michael Niedermayer
michael at niedermayer.cc
Thu Aug 20 23:32:23 EEST 2020
On Thu, Aug 20, 2020 at 12:46:12PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Wed, Aug 19, 2020 at 12:00:37AM +0200, Andreas Rheinhardt wrote:
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> >> ---
> >> The memleak can be reproduced with e.g. the first 163 bytes of
> >> https://samples.ffmpeg.org/archive/all/avi+dvvideo+pcm_s16le++ffmpeg-avidec554-crash.avi
> >>
> >> libavformat/avidec.c | 31 +++++++++++++++++--------------
> >> 1 file changed, 17 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> >> index 5fc3e01aa9..08b864f19a 100644
> >> --- a/libavformat/avidec.c
> >> +++ b/libavformat/avidec.c
> >> @@ -113,6 +113,7 @@ static const AVMetadataConv avi_metadata_conv[] = {
> >> { 0 },
> >> };
> >>
> >> +static int avi_read_close(AVFormatContext *s);
> >> static int avi_load_index(AVFormatContext *s);
> >> static int guess_ni_flag(AVFormatContext *s);
> >>
> >> @@ -464,6 +465,7 @@ static int calculate_bitrate(AVFormatContext *s)
> >> return 1;
> >> }
> >>
> >> +#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
> >> static int avi_read_header(AVFormatContext *s)
> >> {
> >> AVIContext *avi = s->priv_data;
> >> @@ -499,7 +501,7 @@ static int avi_read_header(AVFormatContext *s)
> >> frame_period = 0;
> >> for (;;) {
> >> if (avio_feof(pb))
> >> - goto fail;
> >> + RETURN_ERROR(AVERROR_INVALIDDATA);
> >
> > this macro is messy
> > it replaces writing
> > {ret = AVERROR_INVALIDDATA; goto fail;}
> > by
> > RETURN_ERROR(AVERROR_INVALIDDATA);
> >
> > this is almost the same length but the first is directly understood C code
> > the 2nd is harder to understand for someone reading the code so i
> > suggest to avoid the 2nd and use something else, not saying that needs to
> > be the first
> >
> The only reason this macro exists is because it allows me to add code
> that can easily be removed lateron when cleaning up after read_header
> failure will be automatic, whereas an
>
> if (foo) {
> ret = bar;
> goto fail;
> }
>
> leads to a bigger diff now and later. If you want to, I could of course use
I have no oppinion about the intermediate state but i think we should have
optimally readable/clean code in the end after all planned changes
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200820/dc0b35d2/attachment.sig>
More information about the ffmpeg-devel
mailing list