diff mbox series

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

Message ID 20201028225643.30485-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/4] avformat/apngdec: Check for incomplete reads in append_extradata()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 28, 2020, 10:56 p.m. UTC
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@niedermayer.cc>
---
 libavformat/apngdec.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andreas Rheinhardt Oct. 29, 2020, 1:25 p.m. UTC | #1
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@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.)

- Andreas
Michael Niedermayer Oct. 30, 2020, 10:18 p.m. UTC | #2
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@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

If you prefer your patch from january that should achieve the same.

thx

[...]
Andreas Rheinhardt Oct. 30, 2020, 10:48 p.m. UTC | #3
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@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).

- Andreas
Michael Niedermayer Oct. 31, 2020, 10:35 a.m. UTC | #4
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@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

[...]
diff mbox series

Patch

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