[FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Andreas Rheinhardt]

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 at 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