Re: [FFmpeg-devel] [PATCH 5/9] avformat/mov: Check extend and base offset

[James Almer <jamrial@gmail.com>]

On 6/20/2024 7:54 PM, Michael Niedermayer wrote:
> On Wed, Jun 19, 2024 at 03:08:58PM +0200, Rémi Denis-Courmont wrote:
>>
>>
>> Le 19 juin 2024 14:34:59 GMT+02:00, James Almer <jamrial@gmail.com> a écrit :
>>> On 6/18/2024 4:07 AM, Rémi Denis-Courmont wrote:
>>>>
>>>>
>>>> Le 17 juin 2024 01:08:27 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc> a écrit :
>>>>> Fixes: signed integer overflow: 2314885530818453536 + 9151314442816847872 cannot be represented in type 'long'
>>>>> Fixes: 68359/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-6571950311800832
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>> libavformat/mov.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>>> index 9016cd5ad08..46cbce98040 100644
>>>>> --- a/libavformat/mov.c
>>>>> +++ b/libavformat/mov.c
>>>>> @@ -8131,7 +8131,9 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>>           }
>>>>>           for (int j = 0; j < extent_count; j++) {
>>>>>               if (rb_size(pb, &extent_offset, offset_size) < 0 ||
>>>>> -                rb_size(pb, &extent_length, length_size) < 0)
>>>>> +                rb_size(pb, &extent_length, length_size) < 0 ||
>>>>> +                base_offset < 0 || extent_offset < 0 ||
>>>>> +                base_offset + (uint64_t)extent_offset > INT64_MAX)
>>>>
>>>> Can we please stop with the bespoke arithmetic overflow checks and add dedicated helpers instead, similar to what GCC and C23 have?
>>>
>>> You mean the __builtin_*_overflow() one?
>>
>> I'd rather the ckd_*() stuff but the differences are mostly stylistic.
> 
> Whatever is used must be supported by all currently supported platforms
> that especially also includes past releases we backport things to.
> 
> In practice that means continuing to use the classical way to check
> as well as our av_sat_addXY() stuff.
> 
> We cannot backport things that depend on C23 as that was not a requirement
> in the past. So I also cannot use this in bug fixes.

This change is ok for now (With my suggestion amended to it). We can add 
wrappers around the GCC builtins and C23 ckd_* macros later (With pure C 
fallbacks), and start using them for future changes.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
_______________________________________________
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".