[FFmpeg-devel] Curly braces around single statements (was avformat/avidec: Fix memleak when error) happens after creating DV stream

[Alexander Strasser]

On 2020-08-20 22:32 +0200, Michael Niedermayer wrote:
> 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

Please pardon me for bringing this up in the context of this patch.
No objections or particular opinion regarding this instance of the
problem.

Though thinking more globally, I believe it would have a beneficial
impact to add the curly braces everywhere; even where they would not
be required because of the one-statement exception.

It might be a bit longer regarding vertical space consumption, but
I'm sure the advantages would outweigh this disadvantage over time.
Also because we don't put the opening brace on a line of its own,
it would not consume so much more vertical space.

Advantages I see are:

1. enables easier experimentation and debugging
2. future changes are easier to write and create smaller diffs
3. completely eliminates dangling-else problems


Just wanted to hear how other developers currently feel about this.


  Alexander