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

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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200919/e2c20929/attachment.sig>