* [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
@ 2022-10-02 15:43 Michael Niedermayer
2022-10-02 16:13 ` Rémi Denis-Courmont
0 siblings, 1 reply; 6+ messages in thread
From: Michael Niedermayer @ 2022-10-02 15:43 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented in type 'int'
Fixes: 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481067503616
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
libavcodec/bonk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
index 409694f710d..32f7c9b9bdb 100644
--- a/libavcodec/bonk.c
+++ b/libavcodec/bonk.c
@@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int entries, int base_2_part)
if (!dominant)
n_zeros += steplet;
+ if (step > INT_MAX/9*8)
+ return AVERROR_INVALIDDATA;
+
step += step / 8;
} else if (steplet > 0) {
int actual_run = read_uint_max(s, steplet - 1);
--
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
2022-10-02 15:43 [FFmpeg-devel] [PATCH] avcodec/bonk: Check step Michael Niedermayer
@ 2022-10-02 16:13 ` Rémi Denis-Courmont
2022-10-02 16:26 ` James Almer
2022-10-03 14:49 ` Michael Niedermayer
0 siblings, 2 replies; 6+ messages in thread
From: Rémi Denis-Courmont @ 2022-10-02 16:13 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit :
> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented
> in type 'int' Fixes:
> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481
> 067503616
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavcodec/bonk.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
> index 409694f710d..32f7c9b9bdb 100644
> --- a/libavcodec/bonk.c
> +++ b/libavcodec/bonk.c
> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int
> entries, int base_2_part) if (!dominant)
> n_zeros += steplet;
>
> + if (step > INT_MAX/9*8)
> + return AVERROR_INVALIDDATA;
> +
> step += step / 8;
> } else if (steplet > 0) {
> int actual_run = read_uint_max(s, steplet - 1);
No problem with this patch *specifically* but wouldn't it be more effective to
fix that sort of issue with checked arithmetic, e.g. something like:
if (av_ckd_add(&step, step, step / 8))
return AVERROR_INVALIDDATA;
...especially on 64-bit systems whence this is really just an add. This also
avoids having to figure out what the exact boundary value is.
--
レミ・デニ-クールモン
http://www.remlab.net/
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
2022-10-02 16:13 ` Rémi Denis-Courmont
@ 2022-10-02 16:26 ` James Almer
2022-10-02 16:38 ` Andreas Rheinhardt
2022-10-02 17:02 ` Rémi Denis-Courmont
2022-10-03 14:49 ` Michael Niedermayer
1 sibling, 2 replies; 6+ messages in thread
From: James Almer @ 2022-10-02 16:26 UTC (permalink / raw)
To: ffmpeg-devel
On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote:
> Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit :
>> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented
>> in type 'int' Fixes:
>> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481
>> 067503616
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>> libavcodec/bonk.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
>> index 409694f710d..32f7c9b9bdb 100644
>> --- a/libavcodec/bonk.c
>> +++ b/libavcodec/bonk.c
>> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int
>> entries, int base_2_part) if (!dominant)
>> n_zeros += steplet;
>>
>> + if (step > INT_MAX/9*8)
>> + return AVERROR_INVALIDDATA;
>> +
>> step += step / 8;
>> } else if (steplet > 0) {
>> int actual_run = read_uint_max(s, steplet - 1);
>
> No problem with this patch *specifically* but wouldn't it be more effective to
> fix that sort of issue with checked arithmetic, e.g. something like:
>
> if (av_ckd_add(&step, step, step / 8))
That's __builtin_add_overflow() from gcc/clang.
There's av_sat_add32(), which will clip the result to fit in a 32-bit
variable. So i guess it could be used and then just check for step ==
INT32_MAX and error out, but that's slower than what this patch is doing.
> return AVERROR_INVALIDDATA;
>
> ...especially on 64-bit systems whence this is really just an add. This also
> avoids having to figure out what the exact boundary value is.
What 64-bit arch has sizeof(int) == 8?
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
2022-10-02 16:26 ` James Almer
@ 2022-10-02 16:38 ` Andreas Rheinhardt
2022-10-02 17:02 ` Rémi Denis-Courmont
1 sibling, 0 replies; 6+ messages in thread
From: Andreas Rheinhardt @ 2022-10-02 16:38 UTC (permalink / raw)
To: ffmpeg-devel
James Almer:
> On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote:
>> Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a
>> écrit :
>>> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be
>>> represented
>>> in type 'int' Fixes:
>>> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481
>>> 067503616
>>>
>>> Found-by: continuous fuzzing process
>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>> libavcodec/bonk.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
>>> index 409694f710d..32f7c9b9bdb 100644
>>> --- a/libavcodec/bonk.c
>>> +++ b/libavcodec/bonk.c
>>> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf,
>>> int
>>> entries, int base_2_part) if (!dominant)
>>> n_zeros += steplet;
>>>
>>> + if (step > INT_MAX/9*8)
>>> + return AVERROR_INVALIDDATA;
>>> +
>>> step += step / 8;
>>> } else if (steplet > 0) {
>>> int actual_run = read_uint_max(s, steplet - 1);
>>
>> No problem with this patch *specifically* but wouldn't it be more
>> effective to
>> fix that sort of issue with checked arithmetic, e.g. something like:
>>
>> if (av_ckd_add(&step, step, step / 8))
>
> That's __builtin_add_overflow() from gcc/clang.
>
> There's av_sat_add32(), which will clip the result to fit in a 32-bit
> variable. So i guess it could be used and then just check for step ==
> INT32_MAX and error out, but that's slower than what this patch is doing.
>
The result of av_sat_add32() being INT32_MAX/MIN does not imply that the
result has been saturated. Apart from that, the documentation of
av_sat_add32() pretty much presumes that INT_MIN/MAX == INT32_MIN/MAX.
>> return AVERROR_INVALIDDATA;
>>
>> ...especially on 64-bit systems whence this is really just an add.
>> This also
>> avoids having to figure out what the exact boundary value is.
>
> What 64-bit arch has sizeof(int) == 8?
IIRC on Cray computers all integer types that don't have a size of 1
(character types) have size 8. Notice that there are several places in
our codebase where we pretty much presume int/unsigned to be 32bits (and
to have no padding).
- Andreas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
2022-10-02 16:26 ` James Almer
2022-10-02 16:38 ` Andreas Rheinhardt
@ 2022-10-02 17:02 ` Rémi Denis-Courmont
1 sibling, 0 replies; 6+ messages in thread
From: Rémi Denis-Courmont @ 2022-10-02 17:02 UTC (permalink / raw)
To: ffmpeg-devel
Le sunnuntaina 2. lokakuuta 2022, 19.26.21 EEST James Almer a écrit :
> On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote:
> > Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a
écrit :
> >> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be
> >> represented in type 'int' Fixes:
> >> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-47914
> >> 81
> >> 067503616
> >>
> >> Found-by: continuous fuzzing process
> >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>
> >> libavcodec/bonk.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
> >> index 409694f710d..32f7c9b9bdb 100644
> >> --- a/libavcodec/bonk.c
> >> +++ b/libavcodec/bonk.c
> >> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int
> >> entries, int base_2_part) if (!dominant)
> >>
> >> n_zeros += steplet;
> >>
> >> + if (step > INT_MAX/9*8)
> >> + return AVERROR_INVALIDDATA;
> >> +
> >>
> >> step += step / 8;
> >>
> >> } else if (steplet > 0) {
> >>
> >> int actual_run = read_uint_max(s, steplet - 1);
> >
> > No problem with this patch *specifically* but wouldn't it be more
> > effective to>
> > fix that sort of issue with checked arithmetic, e.g. something like:
> > if (av_ckd_add(&step, step, step / 8))
>
> That's __builtin_add_overflow() from gcc/clang.
I took the name and paramater order ckd_add from Cnext's <stdckdint.h>.
> There's av_sat_add32(), which will clip the result to fit in a 32-bit
> variable. So i guess it could be used and then just check for step ==
> INT32_MAX and error out, but that's slower than what this patch is doing.
Yes but the saturation behaviour is what makes it slower, and it's totally
unnecessary here.
On x86-64, GCC generates this for Michael's code:
cmp $0x71c71c70,%edi
jg 1f
test %edi,%edi
lea 0x7(%rdi),%eax
cmovns %edi,%eax
sar $0x3,%eax
add %edi,%eax
ret
1: cs nopw 0x0(%rax,%rax,1)
mov $0xffffffea,%eax
ret
cs nopw 0x0(%rax,%rax,1)
And this if you use checked overflowing arithmetic:
test %edi,%edi
lea 0x7(%rdi),%eax
mov $0xffffffea,%edx
cmovns %edi,%eax
sar $0x3,%eax
add %edi,%eax
cmovo %edx,%eax
ret
> > return AVERROR_INVALIDDATA;
> >
> > ...especially on 64-bit systems whence this is really just an add. This
> > also avoids having to figure out what the exact boundary value is.
>
> What 64-bit arch has sizeof(int) == 8?
None. But the compiler can freely upgrade the addition to 64-bit internally.
--
雷米‧德尼-库尔蒙
http://www.remlab.net/
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step
2022-10-02 16:13 ` Rémi Denis-Courmont
2022-10-02 16:26 ` James Almer
@ 2022-10-03 14:49 ` Michael Niedermayer
1 sibling, 0 replies; 6+ messages in thread
From: Michael Niedermayer @ 2022-10-03 14:49 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Piotr Bandurski
[-- Attachment #1.1: Type: text/plain, Size: 2614 bytes --]
On Sun, Oct 02, 2022 at 07:13:39PM +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit :
> > Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented
> > in type 'int' Fixes:
> > 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481
> > 067503616
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavcodec/bonk.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
> > index 409694f710d..32f7c9b9bdb 100644
> > --- a/libavcodec/bonk.c
> > +++ b/libavcodec/bonk.c
> > @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int
> > entries, int base_2_part) if (!dominant)
> > n_zeros += steplet;
> >
> > + if (step > INT_MAX/9*8)
if you want the exact limit for any INT_MAX its:
(INT_MAX + 1ULL) / 9 * 8 + (INT_MAX + 1ULL) % 9 - 1;
though probably a fixed bit value would be better anyway for reproducability
> > + return AVERROR_INVALIDDATA;
> > +
> > step += step / 8;
> > } else if (steplet > 0) {
> > int actual_run = read_uint_max(s, steplet - 1);
>
> No problem with this patch *specifically* but wouldn't it be more effective to
> fix that sort of issue with checked arithmetic, e.g. something like:
>
> if (av_ckd_add(&step, step, step / 8))
> return AVERROR_INVALIDDATA;
>
> ...especially on 64-bit systems whence this is really just an add. This also
> avoids having to figure out what the exact boundary value is.
If someone has a set of testfiles for this decoder then I can look at this
and cleanup the code more completely.
i dont know if a step > C vs overflow check is better but i have the suspicion
there are more problems as multiple statments around this seem unreachable.
And iam also missing some check that i would expect to be in there
I found links to datafilehost.com to test files but they are all expired
so i have no test files, so iam a bit cautious with changes ...
CCing piotr who linked to the files on datafilehost, maybe he still has
them.
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
[-- 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".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-03 14:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-02 15:43 [FFmpeg-devel] [PATCH] avcodec/bonk: Check step Michael Niedermayer
2022-10-02 16:13 ` Rémi Denis-Courmont
2022-10-02 16:26 ` James Almer
2022-10-02 16:38 ` Andreas Rheinhardt
2022-10-02 17:02 ` Rémi Denis-Courmont
2022-10-03 14:49 ` Michael Niedermayer
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git