[FFmpeg-devel] [PATCH 1/4] avformat/apngdec: Check for incomplete reads in append_extradata()

[Michael Niedermayer]

On Fri, Oct 30, 2020 at 11:48:11PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Thu, Oct 29, 2020 at 02:25:49PM +0100, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> Fixes: OOM
> >>> Fixes: 26608/clusterfuzz-testcase-minimized-ffmpeg_dem_APNG_fuzzer-4839491644424192
> >>>
> >>> 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/apngdec.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
> >>> index 0f1d04a365..2e79fdd85c 100644
> >>> --- a/libavformat/apngdec.c
> >>> +++ b/libavformat/apngdec.c
> >>> @@ -140,6 +140,8 @@ static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
> >>>  
> >>>      if ((ret = avio_read(pb, par->extradata + previous_size, len)) < 0)
> >>>          return ret;
> >>> +    if (ret < len)
> >>> +        return AVERROR_INVALIDDATA;
> >>>  
> >>>      return previous_size;
> >>>  }
> >>>
> >> Reminds me of
> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255671.html. But
> >> how can this fix an OOM scenario? If avio_read() couldn't read
> >> everything it should read, then we are at the end of the file and the
> >> avio_feof() check will make sure that this is the last iteration of the
> >> loop. Or is this a file that is being written to while it is read? (In
> >> which case an earlier reading attempt might have failed, but a new one
> >> might succeed because there is new data.)
> > 
> > The OOM occurs when the gigiabyte? sized uninitialized extradata is copied
> > and moved around later outside the demuxer
> > 
> 
> Yeah, I've forgotten: In this case the demuxer does not indicate an
> error at all (which is precisely what I wanted to fix).
> 

> > If you prefer your patch from january that should achieve the same.
> > 
> Actually I think one should use ffio_read_size() here (and one should
> also adapt ffio_read_size() to forward the error if avio_read() returned
> an error, unless said error was AVERROR_EOF).

ok, patches doing that posted

thx

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20201031/c7a2275d/attachment.sig>