| Message ID | 20200719174218.30659-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | b78860e769876d9a18fc4f82dd8e808316d8e682 |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avformat/wc3movie: Move wc3_read_close() up | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 7/19/2020 2:42 PM, Michael Niedermayer wrote: > Fixes: memleak > Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c > index c59b5bf6cc..76e945d261 100644 > --- a/libavformat/wc3movie.c > +++ b/libavformat/wc3movie.c > @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) > /* load up the name */ > buffer = av_malloc(size+1); > if (!buffer) > - return AVERROR(ENOMEM); > + if (!buffer) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > if ((ret = avio_read(pb, buffer, size)) != size) { > av_freep(&buffer); > - return AVERROR(EIO); > + ret = AVERROR(EIO); > + goto fail; > } > buffer[size] = 0; > av_dict_set(&s->metadata, "title", buffer, > @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) > default: > av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", > av_fourcc2str(fourcc_tag)); > - return AVERROR_INVALIDDATA; > + ret = AVERROR_INVALIDDATA; > + goto fail; > } > > fourcc_tag = avio_rl32(pb); > /* chunk sizes are 16-bit aligned */ > size = (avio_rb32(pb) + 1) & (~1); > - if (avio_feof(pb)) > - return AVERROR(EIO); > + if (avio_feof(pb)) { > + ret = AVERROR(EIO); > + goto fail; > + } > > } while (fourcc_tag != BRCH_TAG); > > /* initialize the decoder streams */ > st = avformat_new_stream(s, NULL); > - if (!st) > - return AVERROR(ENOMEM); > + if (!st) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > wc3->video_stream_index = st->index; > st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; > @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) > st->codecpar->height = wc3->height; > > st = avformat_new_stream(s, NULL); > - if (!st) > - return AVERROR(ENOMEM); > + if (!st) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > wc3->audio_stream_index = st->index; > st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; > @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) > st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; > > return 0; > +fail: > + wc3_read_close(s); Wouldn't it be better to instead make avformat_open_input() call iformat->read_close() on iformat->read_header() failure? It may require ensuring all demuxers behave nice with it, but the end result would be a lot cleaner. > + return ret; > } > > static int wc3_read_packet(AVFormatContext *s, >
James Almer: > On 7/19/2020 2:42 PM, Michael Niedermayer wrote: >> Fixes: memleak >> Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- >> 1 file changed, 23 insertions(+), 9 deletions(-) >> >> diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c >> index c59b5bf6cc..76e945d261 100644 >> --- a/libavformat/wc3movie.c >> +++ b/libavformat/wc3movie.c >> @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) >> /* load up the name */ >> buffer = av_malloc(size+1); >> if (!buffer) >> - return AVERROR(ENOMEM); >> + if (!buffer) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> if ((ret = avio_read(pb, buffer, size)) != size) { >> av_freep(&buffer); >> - return AVERROR(EIO); >> + ret = AVERROR(EIO); >> + goto fail; >> } >> buffer[size] = 0; >> av_dict_set(&s->metadata, "title", buffer, >> @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) >> default: >> av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", >> av_fourcc2str(fourcc_tag)); >> - return AVERROR_INVALIDDATA; >> + ret = AVERROR_INVALIDDATA; >> + goto fail; >> } >> >> fourcc_tag = avio_rl32(pb); >> /* chunk sizes are 16-bit aligned */ >> size = (avio_rb32(pb) + 1) & (~1); >> - if (avio_feof(pb)) >> - return AVERROR(EIO); >> + if (avio_feof(pb)) { >> + ret = AVERROR(EIO); >> + goto fail; >> + } >> >> } while (fourcc_tag != BRCH_TAG); >> >> /* initialize the decoder streams */ >> st = avformat_new_stream(s, NULL); >> - if (!st) >> - return AVERROR(ENOMEM); >> + if (!st) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); >> wc3->video_stream_index = st->index; >> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; >> @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) >> st->codecpar->height = wc3->height; >> >> st = avformat_new_stream(s, NULL); >> - if (!st) >> - return AVERROR(ENOMEM); >> + if (!st) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); >> wc3->audio_stream_index = st->index; >> st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; >> @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) >> st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; >> >> return 0; >> +fail: >> + wc3_read_close(s); > > Wouldn't it be better to instead make avformat_open_input() call > iformat->read_close() on iformat->read_header() failure? > > It may require ensuring all demuxers behave nice with it, but the end > result would be a lot cleaner. > Problem is: Not all input devices behave nice and it is possible to use an older libavdevice together with a newer libavformat. You might remember the patchset where I added a flag to AVInputFormat for this purpose. I'll resend it soon. - Andreas
Andreas Rheinhardt (12020-07-19): > it is possible to use > an older libavdevice together with a newer libavformat. I move we make it impossible. Regards,
On Sun, Jul 19, 2020 at 02:50:45PM -0300, James Almer wrote: > On 7/19/2020 2:42 PM, Michael Niedermayer wrote: > > Fixes: memleak > > Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c > > index c59b5bf6cc..76e945d261 100644 > > --- a/libavformat/wc3movie.c > > +++ b/libavformat/wc3movie.c > > @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) > > /* load up the name */ > > buffer = av_malloc(size+1); > > if (!buffer) > > - return AVERROR(ENOMEM); > > + if (!buffer) { > > + ret = AVERROR(ENOMEM); > > + goto fail; > > + } > > if ((ret = avio_read(pb, buffer, size)) != size) { > > av_freep(&buffer); > > - return AVERROR(EIO); > > + ret = AVERROR(EIO); > > + goto fail; > > } > > buffer[size] = 0; > > av_dict_set(&s->metadata, "title", buffer, > > @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) > > default: > > av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", > > av_fourcc2str(fourcc_tag)); > > - return AVERROR_INVALIDDATA; > > + ret = AVERROR_INVALIDDATA; > > + goto fail; > > } > > > > fourcc_tag = avio_rl32(pb); > > /* chunk sizes are 16-bit aligned */ > > size = (avio_rb32(pb) + 1) & (~1); > > - if (avio_feof(pb)) > > - return AVERROR(EIO); > > + if (avio_feof(pb)) { > > + ret = AVERROR(EIO); > > + goto fail; > > + } > > > > } while (fourcc_tag != BRCH_TAG); > > > > /* initialize the decoder streams */ > > st = avformat_new_stream(s, NULL); > > - if (!st) > > - return AVERROR(ENOMEM); > > + if (!st) { > > + ret = AVERROR(ENOMEM); > > + goto fail; > > + } > > avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > > wc3->video_stream_index = st->index; > > st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; > > @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) > > st->codecpar->height = wc3->height; > > > > st = avformat_new_stream(s, NULL); > > - if (!st) > > - return AVERROR(ENOMEM); > > + if (!st) { > > + ret = AVERROR(ENOMEM); > > + goto fail; > > + } > > avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > > wc3->audio_stream_index = st->index; > > st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; > > @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) > > st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; > > > > return 0; > > +fail: > > + wc3_read_close(s); > > Wouldn't it be better to instead make avformat_open_input() call > iformat->read_close() on iformat->read_header() failure? > > It may require ensuring all demuxers behave nice with it, > but the end > result would be a lot cleaner. yes, certainly. [...]
On Sun, Jul 19, 2020 at 07:55:24PM +0200, Andreas Rheinhardt wrote: > James Almer: > > On 7/19/2020 2:42 PM, Michael Niedermayer wrote: > >> Fixes: memleak > >> Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- > >> 1 file changed, 23 insertions(+), 9 deletions(-) > >> > >> diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c > >> index c59b5bf6cc..76e945d261 100644 > >> --- a/libavformat/wc3movie.c > >> +++ b/libavformat/wc3movie.c > >> @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) > >> /* load up the name */ > >> buffer = av_malloc(size+1); > >> if (!buffer) > >> - return AVERROR(ENOMEM); > >> + if (!buffer) { > >> + ret = AVERROR(ENOMEM); > >> + goto fail; > >> + } > >> if ((ret = avio_read(pb, buffer, size)) != size) { > >> av_freep(&buffer); > >> - return AVERROR(EIO); > >> + ret = AVERROR(EIO); > >> + goto fail; > >> } > >> buffer[size] = 0; > >> av_dict_set(&s->metadata, "title", buffer, > >> @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) > >> default: > >> av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", > >> av_fourcc2str(fourcc_tag)); > >> - return AVERROR_INVALIDDATA; > >> + ret = AVERROR_INVALIDDATA; > >> + goto fail; > >> } > >> > >> fourcc_tag = avio_rl32(pb); > >> /* chunk sizes are 16-bit aligned */ > >> size = (avio_rb32(pb) + 1) & (~1); > >> - if (avio_feof(pb)) > >> - return AVERROR(EIO); > >> + if (avio_feof(pb)) { > >> + ret = AVERROR(EIO); > >> + goto fail; > >> + } > >> > >> } while (fourcc_tag != BRCH_TAG); > >> > >> /* initialize the decoder streams */ > >> st = avformat_new_stream(s, NULL); > >> - if (!st) > >> - return AVERROR(ENOMEM); > >> + if (!st) { > >> + ret = AVERROR(ENOMEM); > >> + goto fail; > >> + } > >> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > >> wc3->video_stream_index = st->index; > >> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; > >> @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) > >> st->codecpar->height = wc3->height; > >> > >> st = avformat_new_stream(s, NULL); > >> - if (!st) > >> - return AVERROR(ENOMEM); > >> + if (!st) { > >> + ret = AVERROR(ENOMEM); > >> + goto fail; > >> + } > >> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > >> wc3->audio_stream_index = st->index; > >> st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; > >> @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) > >> st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; > >> > >> return 0; > >> +fail: > >> + wc3_read_close(s); > > > > Wouldn't it be better to instead make avformat_open_input() call > > iformat->read_close() on iformat->read_header() failure? > > > > It may require ensuring all demuxers behave nice with it, but the end > > result would be a lot cleaner. > > > > Problem is: Not all input devices behave nice and it is possible to use > an older libavdevice together with a newer libavformat. You might > remember the patchset where I added a flag to AVInputFormat for this > purpose. I'll resend it soon. 2 months have passed, the memleak is still open and i dont see a flag or init/deinit() for demuxers. So i suggest to apply this patch as it was. A flag is better but a leak is worst. thx [...]
Michael Niedermayer: > On Sun, Jul 19, 2020 at 07:55:24PM +0200, Andreas Rheinhardt wrote: >> James Almer: >>> On 7/19/2020 2:42 PM, Michael Niedermayer wrote: >>>> Fixes: memleak >>>> Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 >>>> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- >>>> 1 file changed, 23 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c >>>> index c59b5bf6cc..76e945d261 100644 >>>> --- a/libavformat/wc3movie.c >>>> +++ b/libavformat/wc3movie.c >>>> @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) >>>> /* load up the name */ >>>> buffer = av_malloc(size+1); >>>> if (!buffer) >>>> - return AVERROR(ENOMEM); >>>> + if (!buffer) { >>>> + ret = AVERROR(ENOMEM); >>>> + goto fail; >>>> + } >>>> if ((ret = avio_read(pb, buffer, size)) != size) { >>>> av_freep(&buffer); >>>> - return AVERROR(EIO); >>>> + ret = AVERROR(EIO); >>>> + goto fail; >>>> } >>>> buffer[size] = 0; >>>> av_dict_set(&s->metadata, "title", buffer, >>>> @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) >>>> default: >>>> av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", >>>> av_fourcc2str(fourcc_tag)); >>>> - return AVERROR_INVALIDDATA; >>>> + ret = AVERROR_INVALIDDATA; >>>> + goto fail; >>>> } >>>> >>>> fourcc_tag = avio_rl32(pb); >>>> /* chunk sizes are 16-bit aligned */ >>>> size = (avio_rb32(pb) + 1) & (~1); >>>> - if (avio_feof(pb)) >>>> - return AVERROR(EIO); >>>> + if (avio_feof(pb)) { >>>> + ret = AVERROR(EIO); >>>> + goto fail; >>>> + } >>>> >>>> } while (fourcc_tag != BRCH_TAG); >>>> >>>> /* initialize the decoder streams */ >>>> st = avformat_new_stream(s, NULL); >>>> - if (!st) >>>> - return AVERROR(ENOMEM); >>>> + if (!st) { >>>> + ret = AVERROR(ENOMEM); >>>> + goto fail; >>>> + } >>>> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); >>>> wc3->video_stream_index = st->index; >>>> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; >>>> @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) >>>> st->codecpar->height = wc3->height; >>>> >>>> st = avformat_new_stream(s, NULL); >>>> - if (!st) >>>> - return AVERROR(ENOMEM); >>>> + if (!st) { >>>> + ret = AVERROR(ENOMEM); >>>> + goto fail; >>>> + } >>>> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); >>>> wc3->audio_stream_index = st->index; >>>> st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; >>>> @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) >>>> st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; >>>> >>>> return 0; >>>> +fail: >>>> + wc3_read_close(s); >>> >>> Wouldn't it be better to instead make avformat_open_input() call >>> iformat->read_close() on iformat->read_header() failure? >>> >>> It may require ensuring all demuxers behave nice with it, but the end >>> result would be a lot cleaner. >>> >> >> Problem is: Not all input devices behave nice and it is possible to use >> an older libavdevice together with a newer libavformat. You might >> remember the patchset where I added a flag to AVInputFormat for this >> purpose. I'll resend it soon. > > 2 months have passed, the memleak is still open and i dont see a flag or > init/deinit() for demuxers. > So i suggest to apply this patch as it was. A flag is better but a leak is > worst. > I agree. - Andreas
On Sat, Sep 19, 2020 at 10:34:46AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Sun, Jul 19, 2020 at 07:55:24PM +0200, Andreas Rheinhardt wrote: > >> James Almer: > >>> On 7/19/2020 2:42 PM, Michael Niedermayer wrote: > >>>> Fixes: memleak > >>>> Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 > >>>> > >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>> --- > >>>> libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- > >>>> 1 file changed, 23 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c > >>>> index c59b5bf6cc..76e945d261 100644 > >>>> --- a/libavformat/wc3movie.c > >>>> +++ b/libavformat/wc3movie.c > >>>> @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) > >>>> /* load up the name */ > >>>> buffer = av_malloc(size+1); > >>>> if (!buffer) > >>>> - return AVERROR(ENOMEM); > >>>> + if (!buffer) { > >>>> + ret = AVERROR(ENOMEM); > >>>> + goto fail; > >>>> + } > >>>> if ((ret = avio_read(pb, buffer, size)) != size) { > >>>> av_freep(&buffer); > >>>> - return AVERROR(EIO); > >>>> + ret = AVERROR(EIO); > >>>> + goto fail; > >>>> } > >>>> buffer[size] = 0; > >>>> av_dict_set(&s->metadata, "title", buffer, > >>>> @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) > >>>> default: > >>>> av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", > >>>> av_fourcc2str(fourcc_tag)); > >>>> - return AVERROR_INVALIDDATA; > >>>> + ret = AVERROR_INVALIDDATA; > >>>> + goto fail; > >>>> } > >>>> > >>>> fourcc_tag = avio_rl32(pb); > >>>> /* chunk sizes are 16-bit aligned */ > >>>> size = (avio_rb32(pb) + 1) & (~1); > >>>> - if (avio_feof(pb)) > >>>> - return AVERROR(EIO); > >>>> + if (avio_feof(pb)) { > >>>> + ret = AVERROR(EIO); > >>>> + goto fail; > >>>> + } > >>>> > >>>> } while (fourcc_tag != BRCH_TAG); > >>>> > >>>> /* initialize the decoder streams */ > >>>> st = avformat_new_stream(s, NULL); > >>>> - if (!st) > >>>> - return AVERROR(ENOMEM); > >>>> + if (!st) { > >>>> + ret = AVERROR(ENOMEM); > >>>> + goto fail; > >>>> + } > >>>> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > >>>> wc3->video_stream_index = st->index; > >>>> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; > >>>> @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) > >>>> st->codecpar->height = wc3->height; > >>>> > >>>> st = avformat_new_stream(s, NULL); > >>>> - if (!st) > >>>> - return AVERROR(ENOMEM); > >>>> + if (!st) { > >>>> + ret = AVERROR(ENOMEM); > >>>> + goto fail; > >>>> + } > >>>> avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); > >>>> wc3->audio_stream_index = st->index; > >>>> st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; > >>>> @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) > >>>> st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; > >>>> > >>>> return 0; > >>>> +fail: > >>>> + wc3_read_close(s); > >>> > >>> Wouldn't it be better to instead make avformat_open_input() call > >>> iformat->read_close() on iformat->read_header() failure? > >>> > >>> It may require ensuring all demuxers behave nice with it, but the end > >>> result would be a lot cleaner. > >>> > >> > >> Problem is: Not all input devices behave nice and it is possible to use > >> an older libavdevice together with a newer libavformat. You might > >> remember the patchset where I added a flag to AVInputFormat for this > >> purpose. I'll resend it soon. > > > > 2 months have passed, the memleak is still open and i dont see a flag or > > init/deinit() for demuxers. > > So i suggest to apply this patch as it was. A flag is better but a leak is > > worst. > > > I agree. will apply thx [...]
diff --git a/libavformat/wc3movie.c b/libavformat/wc3movie.c index c59b5bf6cc..76e945d261 100644 --- a/libavformat/wc3movie.c +++ b/libavformat/wc3movie.c @@ -139,10 +139,14 @@ static int wc3_read_header(AVFormatContext *s) /* load up the name */ buffer = av_malloc(size+1); if (!buffer) - return AVERROR(ENOMEM); + if (!buffer) { + ret = AVERROR(ENOMEM); + goto fail; + } if ((ret = avio_read(pb, buffer, size)) != size) { av_freep(&buffer); - return AVERROR(EIO); + ret = AVERROR(EIO); + goto fail; } buffer[size] = 0; av_dict_set(&s->metadata, "title", buffer, @@ -164,21 +168,26 @@ static int wc3_read_header(AVFormatContext *s) default: av_log(s, AV_LOG_ERROR, "unrecognized WC3 chunk: %s\n", av_fourcc2str(fourcc_tag)); - return AVERROR_INVALIDDATA; + ret = AVERROR_INVALIDDATA; + goto fail; } fourcc_tag = avio_rl32(pb); /* chunk sizes are 16-bit aligned */ size = (avio_rb32(pb) + 1) & (~1); - if (avio_feof(pb)) - return AVERROR(EIO); + if (avio_feof(pb)) { + ret = AVERROR(EIO); + goto fail; + } } while (fourcc_tag != BRCH_TAG); /* initialize the decoder streams */ st = avformat_new_stream(s, NULL); - if (!st) - return AVERROR(ENOMEM); + if (!st) { + ret = AVERROR(ENOMEM); + goto fail; + } avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); wc3->video_stream_index = st->index; st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; @@ -188,8 +197,10 @@ static int wc3_read_header(AVFormatContext *s) st->codecpar->height = wc3->height; st = avformat_new_stream(s, NULL); - if (!st) - return AVERROR(ENOMEM); + if (!st) { + ret = AVERROR(ENOMEM); + goto fail; + } avpriv_set_pts_info(st, 33, 1, WC3_FRAME_FPS); wc3->audio_stream_index = st->index; st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; @@ -204,6 +215,9 @@ static int wc3_read_header(AVFormatContext *s) st->codecpar->block_align = WC3_AUDIO_BITS * WC3_AUDIO_CHANNELS; return 0; +fail: + wc3_read_close(s); + return ret; } static int wc3_read_packet(AVFormatContext *s,
Fixes: memleak Fixes: 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/wc3movie.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)