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

[Andreas Rheinhardt]

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 at 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