Re: [FFmpeg-devel] [PATCH 05/11] avcodec/flacdec: Fix signed integre overflow

[James Almer <jamrial@gmail.com>]

On 5/6/2023 12:08 PM, Michael Niedermayer wrote:
> On Fri, May 05, 2023 at 07:36:05PM -0300, James Almer wrote:
>> On 4/16/2023 1:48 PM, Michael Niedermayer wrote:
>>> Fixes: signed integer overflow: 3011809745540902265 + 6323452730883571725 cannot be represented in type 'long'
>>> Fixes: 45982/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-6687553022722048
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavcodec/flacdec.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
>>> index cc778a8dff1..524a0469495 100644
>>> --- a/libavcodec/flacdec.c
>>> +++ b/libavcodec/flacdec.c
>>> @@ -513,7 +513,7 @@ static int decode_subframe_lpc_33bps(FLACContext *s, int64_t *decoded,
>>>        for (i = pred_order; i < s->blocksize; i++, decoded++) {
>>>            int64_t sum = 0;
>>>            for (j = 0; j < pred_order; j++)
>>> -            sum += (int64_t)coeffs[j] * decoded[j];
>>> +            sum += (int64_t)coeffs[j] * (uint64_t)decoded[j];
>>
>> Why not instead do
>>
>> sum = av_sat_add64(sum, (int64_t)coeffs[j] * decoded[j]);
> 
> Why should this be clipping ?
> flac is a lossless codec, i see nothing in the specification that calls for
> cliping.

No, but an overflowing case like 3011809745540902265 + 
6323452730883571725 isn't supported either and will generate bad output, 
so might as well use an optimized function for this.

> 
> 
>>
>> Also, decoded[j] is an int64_t, so wouldn't coeffs[j] be promoted if you
>> swap the order in the multiplication, thus saving the cast?
> 
> it can be shuffled around to achieve the same, do you prefer
>   coeffs[j] * (uint64_t)decoded[j]; ?

No gain doing that compared to your first version. I suggested it for 
av_sat_add64(), which i insist you should use, if you want with a 
comment about it being there not for spec reasons but to prevent integer 
overflows, and removing all casts.

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