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

[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.

> +    return ret;
>  }
>  
>  static int wc3_read_packet(AVFormatContext *s,
>