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

[Andreas Rheinhardt]

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.

- Andreas