| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 [...]
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
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 [...]
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
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
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 --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)
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(-)