diff mbox

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

Message ID 20191125224717.26174-3-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 25, 2019, 10:47 p.m. UTC
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@niedermayer.cc>
---
 libavformat/vividas.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Nov. 28, 2019, 2:45 p.m. UTC | #1
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@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
diff mbox

Patch

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;
     }