[FFmpeg-devel] [PATCH 3/3] avformat/vividas: Free pb on all error paths in track_header()

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: memleak
> Fixes: 19054/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5673287506198528
> 
> 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/vividas.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> index f20af3d7c2..2c6a3daca7 100644
> --- a/libavformat/vividas.c
> +++ b/libavformat/vividas.c
> @@ -296,8 +296,10 @@ static int track_header(VividasDemuxContext *viv, AVFormatContext *s,  uint8_t *
>      for (i=0;i<val_1;i++) {
>          int c = avio_r8(pb);
>          for (j=0;j<c;j++) {
> -            if (avio_feof(pb))
> +            if (avio_feof(pb)) {
> +                av_free(pb);
>                  return AVERROR_EOF;
> +            }
>              avio_r8(pb); // val_3
>              avio_r8(pb); // val_4
>          }
> @@ -314,6 +316,7 @@ static int track_header(VividasDemuxContext *viv, AVFormatContext *s,  uint8_t *
>      avio_seek(pb, off, SEEK_SET);
>      if (num_video != 1) {
>          av_log(s, AV_LOG_ERROR, "number of video tracks %d is not 1\n", num_video);
> +        av_free(pb);
>          return AVERROR_PATCHWELCOME;
>      }
>  

A better solution for this is to not allocate the AVIOContext at all:
Its lifetime does not extend beyond the function it is used in. I
prepared a patch for this [1]. Given that vividas isn't covered by
FATE (apart from the probe function) my patch is untested. But it
should fix your fuzzing memleak. It will also imply that checking
whether avformat_new_stream() failed as is done in [2] won't produce
memleaks.

- Andreas

[1]: https://patchwork.ffmpeg.org/patch/16473/
[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/253551.html