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

[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.

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