| 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() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
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
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 [...]
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
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 --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; }
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(+)