diff mbox series

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

Message ID 20200818220037.10562-1-andreas.rheinhardt@gmail.com
State Accepted
Commit ea45d6e61a8562fa8094499d2b052ba2e3ce8f6b
Headers show
Series [FFmpeg-devel] avformat/avidec: Fix memleak when error happens after creating DV stream
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 18, 2020, 10 p.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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(-)

Comments

Michael Niedermayer Aug. 20, 2020, 10:30 a.m. UTC | #1
On Wed, Aug 19, 2020 at 12:00:37AM +0200, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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

thanks


[...]
Andreas Rheinhardt Aug. 20, 2020, 10:46 a.m. UTC | #2
Michael Niedermayer:
> On Wed, Aug 19, 2020 at 12:00:37AM +0200, Andreas Rheinhardt wrote:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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
if (foo)
    { ret = bar; goto fail; }

- Andreas
Michael Niedermayer Aug. 20, 2020, 8:32 p.m. UTC | #3
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@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

[...]
Alexander Strasser Aug. 20, 2020, 9:20 p.m. UTC | #4
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@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
Mark Thompson Aug. 20, 2020, 10:35 p.m. UTC | #5
On 20/08/2020 22:20, Alexander Strasser wrote:
> 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

In my opinion this is not a significant benefit, the overhead of any edits while doing this is trivial.  (Others may disagree, but it seems worth noting which parts I agree with.)

> 2. future changes are easier to write and create smaller diffs

I agree that this is a point in favour.

> 3. completely eliminates dangling-else problems

I think this is irrelevant, because compilers have already solved it:

$ cat test-dangling-else.c

int f(int a, int b)
{
     if (a)
         if (b)
             return 1;
       else
           return 2;

     return 0;
}
$ gcc -c -Wall test-dangling-else.c
test-dangling-else.c: In function ‘f’:
test-dangling-else.c:4:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
     4 |     if (a)
       |        ^

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

The freeform nature of C is often helpful to make code look nicer and such a requirement would make some useful patterns worse, so I oppose such a change.

To offer an example, consider the commonish code pattern:

if      (a) p = x;
else if (b) q = y;
else if (c) r = z;

with some alignment of related subexpressions.  Making uniform use of braces and newlines mandatory makes this actively worse however you do it.

(Some randomly-chosen similar examples: <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/mpegaudiodsp_template.c;h=e531f8a904b14ad2f5cc6e59ad608bfe64b50065;hb=HEAD#l236>, <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/error_resilience.c;h=ca2287198be69505a03f12dcbe9cff2b485de769;hb=HEAD#l465>.)

- Mark
Alexander Strasser Aug. 21, 2020, 10:50 a.m. UTC | #6
Hi Mark,

thanks for your opinion on this!

On 2020-08-20 23:35 +0100, Mark Thompson wrote:
> On 20/08/2020 22:20, Alexander Strasser wrote:
> > 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
>
> In my opinion this is not a significant benefit, the overhead of any edits while doing this is trivial.  (Others may disagree, but it seems worth noting which parts I agree with.)

I obviously disagree here, but it's sure a subjective topic.


> > 2. future changes are easier to write and create smaller diffs
>
> I agree that this is a point in favour.
>
> > 3. completely eliminates dangling-else problems
>
> I think this is irrelevant, because compilers have already solved it:
>
> $ cat test-dangling-else.c
>
> int f(int a, int b)
> {
>     if (a)
>         if (b)
>             return 1;
>       else
>           return 2;
>
>     return 0;
> }
> $ gcc -c -Wall test-dangling-else.c
> test-dangling-else.c: In function ‘f’:
> test-dangling-else.c:4:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
>     4 |     if (a)
>       |        ^

Agreed, compilers warn about this. I  still think there is merit in
being able sort it out, before compiling, especially when doing code
reviews on the mailing list.


> > Just wanted to hear how other developers currently feel about this.
>
> The freeform nature of C is often helpful to make code look nicer and such a requirement would make some useful patterns worse, so I oppose such a change.
>
> To offer an example, consider the commonish code pattern:
>
> if      (a) p = x;
> else if (b) q = y;
> else if (c) r = z;

I think this sort of alignment is another practice that didn't stand
the test of time.

I know it is very tempting to use when initially adding some code,
but it is often subjective and hard to keep in shape after (multiple)
change over times. You need to follow up with cosmetics and I assume
more often than not it will be overseen or forgotten, which makes
that code weird to read.

I know this sort of alignment is used often throughout FFmpeg's
code base. If you want I can try to dig out examples where it has
gone wrong, stumbled over this every now and then.


  Alexander

>
> with some alignment of related subexpressions.  Making uniform use of braces and newlines mandatory makes this actively worse however you do it.
>
> (Some randomly-chosen similar examples: <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/mpegaudiodsp_template.c;h=e531f8a904b14ad2f5cc6e59ad608bfe64b50065;hb=HEAD#l236>, <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/error_resilience.c;h=ca2287198be69505a03f12dcbe9cff2b485de769;hb=HEAD#l465>.)
diff mbox series

Patch

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);
         tag  = avio_rl32(pb);
         size = avio_rl32(pb);
 
@@ -571,12 +573,12 @@  static int avi_read_header(AVFormatContext *s)
                 stream_index++;
                 st = avformat_new_stream(s, NULL);
                 if (!st)
-                    goto fail;
+                    RETURN_ERROR(AVERROR(ENOMEM));
 
                 st->id = stream_index;
                 ast    = av_mallocz(sizeof(AVIStream));
                 if (!ast)
-                    goto fail;
+                    RETURN_ERROR(AVERROR(ENOMEM));
                 st->priv_data = ast;
             }
             if (amv_file_format)
@@ -592,12 +594,12 @@  static int avi_read_header(AVFormatContext *s)
                 /* After some consideration -- I don't think we
                  * have to support anything but DV in type1 AVIs. */
                 if (s->nb_streams != 1)
-                    goto fail;
+                    RETURN_ERROR(AVERROR_INVALIDDATA);
 
                 if (handler != MKTAG('d', 'v', 's', 'd') &&
                     handler != MKTAG('d', 'v', 'h', 'd') &&
                     handler != MKTAG('d', 'v', 's', 'l'))
-                    goto fail;
+                    return AVERROR_INVALIDDATA;
 
                 if (!CONFIG_DV_DEMUXER)
                     return AVERROR_DEMUXER_NOT_FOUND;
@@ -697,7 +699,7 @@  static int avi_read_header(AVFormatContext *s)
                            "Invalid sample_size %d at stream %d\n",
                            ast->sample_size,
                            stream_index);
-                    goto fail;
+                    RETURN_ERROR(AVERROR_INVALIDDATA);
                 }
                 av_log(s, AV_LOG_WARNING,
                        "Invalid sample_size %d at stream %d "
@@ -927,7 +929,7 @@  static int avi_read_header(AVFormatContext *s)
                         av_log(s, AV_LOG_WARNING, "New extradata in strd chunk, freeing previous one.\n");
                     }
                     if ((ret = ff_get_extradata(s, st->codecpar, pb, size)) < 0)
-                        return ret;
+                        goto fail;
                 }
 
                 if (st->codecpar->extradata_size & 1) //FIXME check if the encoder really did this correctly
@@ -945,7 +947,7 @@  static int avi_read_header(AVFormatContext *s)
                 avi->use_odml &&
                 read_odml_index(s, 0) < 0 &&
                 (s->error_recognition & AV_EF_EXPLODE))
-                goto fail;
+                RETURN_ERROR(AVERROR_INVALIDDATA);
             avio_seek(pb, pos + size, SEEK_SET);
             break;
         case MKTAG('v', 'p', 'r', 'p'):
@@ -980,7 +982,7 @@  static int avi_read_header(AVFormatContext *s)
             if (s->nb_streams) {
                 ret = avi_read_tag(s, s->streams[s->nb_streams - 1], tag, size);
                 if (ret < 0)
-                    return ret;
+                    goto fail;
                 break;
             }
         default:
@@ -991,7 +993,7 @@  static int avi_read_header(AVFormatContext *s)
                        "I will ignore it and try to continue anyway.\n",
                        av_fourcc2str(tag), size);
                 if (s->error_recognition & AV_EF_EXPLODE)
-                    goto fail;
+                    RETURN_ERROR(AVERROR_INVALIDDATA);
                 avi->movi_list = avio_tell(pb) - 4;
                 avi->movi_end  = avi->fsize;
                 goto end_of_header;
@@ -1008,9 +1010,7 @@  static int avi_read_header(AVFormatContext *s)
 end_of_header:
     /* check stream number */
     if (stream_index != s->nb_streams - 1) {
-
-fail:
-        return AVERROR_INVALIDDATA;
+        RETURN_ERROR(AVERROR_INVALIDDATA);
     }
 
     if (!avi->index_loaded && (pb->seekable & AVIO_SEEKABLE_NORMAL))
@@ -1019,7 +1019,7 @@  fail:
     avi->index_loaded    |= 1;
 
     if ((ret = guess_ni_flag(s)) < 0)
-        return ret;
+        goto fail;
 
     avi->non_interleaved |= ret | (s->flags & AVFMT_FLAG_SORT_DTS);
 
@@ -1056,6 +1056,9 @@  fail:
     ff_metadata_conv_ctx(s, NULL, ff_riff_info_conv);
 
     return 0;
+fail:
+    avi_read_close(s);
+    return ret;
 }
 
 static int read_gab2_sub(AVFormatContext *s, AVStream *st, AVPacket *pkt)