diff mbox series

[FFmpeg-devel,2/6] avformat/wc3movie: Cleanup on wc3_read_header() failure

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
Related show

Checks

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

Commit Message

Michael Niedermayer July 19, 2020, 5:42 p.m. UTC
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(-)

Comments

James Almer July 19, 2020, 5:50 p.m. UTC | #1
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,
>
Andreas Rheinhardt July 19, 2020, 5:55 p.m. UTC | #2
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
Nicolas George July 19, 2020, 5:57 p.m. UTC | #3
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,
Michael Niedermayer July 19, 2020, 8:13 p.m. UTC | #4
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.

[...]
Michael Niedermayer Sept. 19, 2020, 8:31 a.m. UTC | #5
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

[...]
Andreas Rheinhardt Sept. 19, 2020, 8:34 a.m. UTC | #6
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
Michael Niedermayer Sept. 20, 2020, 4:01 p.m. UTC | #7
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 mbox series

Patch

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,