[FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[Michael Niedermayer]

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;
                 if (r &&
                     a->segments[r].start < a->segments[r-1].end &&
                     a->segments[r].end   > a->segments[r-1].start)
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[Anton Khirnov]

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?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[James Almer]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1836 bytes --]

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

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/aaxdec: Check for empty segments

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1568 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".