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

[Michael Niedermayer]

On Sat, Sep 19, 2020 at 10:34:46AM +0200, Andreas Rheinhardt wrote:
> 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.
> > 
> I agree.

will apply

thx

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20200920/374b4dfd/attachment.sig>