[FFmpeg-devel] [PATCH] avformat/avidec: Fix memleak when error happens after creating DV stream

[Michael Niedermayer]

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>