| Message ID | 20220627084347.30362-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/aaxdec: Check for empty segments | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Quoting Michael Niedermayer (2022-06-27 10:43:47) > Fixes: Timeout > Fixes: 48154/clusterfuzz-testcase-minimized-ffmpeg_dem_AAX_fuzzer-5149094353436672 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/aaxdec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c > index dd1fbde736..bcbff216db 100644 > --- a/libavformat/aaxdec.c > +++ b/libavformat/aaxdec.c > @@ -252,6 +252,8 @@ static int aax_read_header(AVFormatContext *s) > size = avio_rb32(pb); > a->segments[r].start = start + a->data_offset; > a->segments[r].end = a->segments[r].start + size; > + if (!size) > + return AVERROR_INVALIDDATA; Why check for invalid size only after some things are set based on it and not before?
On 6/28/2022 2:21 AM, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-06-27 10:43:47) >> Fixes: Timeout >> Fixes: 48154/clusterfuzz-testcase-minimized-ffmpeg_dem_AAX_fuzzer-5149094353436672 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/aaxdec.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c >> index dd1fbde736..bcbff216db 100644 >> --- a/libavformat/aaxdec.c >> +++ b/libavformat/aaxdec.c >> @@ -252,6 +252,8 @@ static int aax_read_header(AVFormatContext *s) >> size = avio_rb32(pb); >> a->segments[r].start = start + a->data_offset; >> a->segments[r].end = a->segments[r].start + size; >> + if (!size) >> + return AVERROR_INVALIDDATA; > > Why check for invalid size only after some things are set based on it > and not before? Also, if the problem is that a->segments[r].start == a->segments[r].end, then maybe it'd be better, or at least more clear to the reader, to ensure that as part of the checks immediately after this line.
On Tue, Jun 28, 2022 at 08:26:54AM -0300, James Almer wrote: > > > On 6/28/2022 2:21 AM, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-06-27 10:43:47) > > > Fixes: Timeout > > > Fixes: 48154/clusterfuzz-testcase-minimized-ffmpeg_dem_AAX_fuzzer-5149094353436672 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/aaxdec.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c > > > index dd1fbde736..bcbff216db 100644 > > > --- a/libavformat/aaxdec.c > > > +++ b/libavformat/aaxdec.c > > > @@ -252,6 +252,8 @@ static int aax_read_header(AVFormatContext *s) > > > size = avio_rb32(pb); > > > a->segments[r].start = start + a->data_offset; > > > a->segments[r].end = a->segments[r].start + size; > > > + if (!size) > > > + return AVERROR_INVALIDDATA; > > > > Why check for invalid size only after some things are set based on it > > and not before? moved it up > > Also, if the problem is that a->segments[r].start == a->segments[r].end, > then maybe it'd be better, or at least more clear to the reader, to ensure > that as part of the checks immediately after this line. its easy to add it into that check but the check becomes more complex and harder to understand. Unless you have some specific suggestion i would tend to keep the if()s seperate thx [...]
On Tue, Jun 28, 2022 at 09:28:30PM +0200, Michael Niedermayer wrote: > On Tue, Jun 28, 2022 at 08:26:54AM -0300, James Almer wrote: > > > > > > On 6/28/2022 2:21 AM, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2022-06-27 10:43:47) > > > > Fixes: Timeout > > > > Fixes: 48154/clusterfuzz-testcase-minimized-ffmpeg_dem_AAX_fuzzer-5149094353436672 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/aaxdec.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c > > > > index dd1fbde736..bcbff216db 100644 > > > > --- a/libavformat/aaxdec.c > > > > +++ b/libavformat/aaxdec.c > > > > @@ -252,6 +252,8 @@ static int aax_read_header(AVFormatContext *s) > > > > size = avio_rb32(pb); > > > > a->segments[r].start = start + a->data_offset; > > > > a->segments[r].end = a->segments[r].start + size; > > > > + if (!size) > > > > + return AVERROR_INVALIDDATA; > > > > > > Why check for invalid size only after some things are set based on it > > > and not before? > > moved it up will apply [...]
diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c index dd1fbde736..bcbff216db 100644 --- a/libavformat/aaxdec.c +++ b/libavformat/aaxdec.c @@ -252,6 +252,8 @@ static int aax_read_header(AVFormatContext *s) size = avio_rb32(pb); a->segments[r].start = start + a->data_offset; a->segments[r].end = a->segments[r].start + size; + if (!size) + return AVERROR_INVALIDDATA; if (r && a->segments[r].start < a->segments[r-1].end && a->segments[r].end > a->segments[r-1].start)
Fixes: Timeout Fixes: 48154/clusterfuzz-testcase-minimized-ffmpeg_dem_AAX_fuzzer-5149094353436672 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/aaxdec.c | 2 ++ 1 file changed, 2 insertions(+)