Re: [FFmpeg-devel] [PATCH] avcodec/jpeg2000dsp: Use unsigned to avoid overflow

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Tomas Härdin:
> tis 2022-09-27 klockan 03:47 +0200 skrev Andreas Rheinhardt:
>> Affected the jpeg2000dsp checkasm test.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/jpeg2000dsp.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/jpeg2000dsp.c b/libavcodec/jpeg2000dsp.c
>> index b61be3b72f..b1bff6d5b1 100644
>> --- a/libavcodec/jpeg2000dsp.c
>> +++ b/libavcodec/jpeg2000dsp.c
>> @@ -76,14 +76,13 @@ static void ict_int(void *_src0, void *_src1,
>> void *_src2, int csize)
>>  
>>  static void rct_int(void *_src0, void *_src1, void *_src2, int
>> csize)
>>  {
>> -    int32_t *src0 = _src0, *src1 = _src1, *src2 = _src2;
>> -    int32_t i0, i1, i2;
>> +    uint32_t *src0 = _src0, *src1 = _src1, *src2 = _src2;
>>      int i;
>>  
>>      for (i = 0; i < csize; i++) {
>> -        i1 = *src0 - (*src2 + *src1 >> 2);
>> -        i0 = i1 + *src2;
>> -        i2 = i1 + *src1;
>> +        uint32_t i1 = *src0 - ((int32_t)(*src2 + *src1) >> 2);
> 
> The addition could conceivably overflow. Also could just use / 4
> instead of >> 2.

The addition uses unsigned types, so that overflow is defined. (I now
notice that my commit message is slightly confusing in this regard: It
uses the spec verbiage which is that arithmetic on unsigned integer
types can never overflow, because it is performed modulo the max of said
type + 1; but it is nevertheless common to still call this overflow.)

Furthermore, the shift is performed on signed types and the rounding for
negative numbers divided by four is different than what >> 2 produces
(integer division is defined to use rounding towards zero, whereas right
shifts of negative numbers are implementation defined and typically use
rounding towards -inf (we require this behaviour)). The test fails if I
use / 4 here (with or without the cast to int32_t).

> 
>> +        int32_t i0 = i1 + *src2;
>> +        int32_t i2 = i1 + *src1;
> 
> These could also overflow. And agian, not in typical use obviously
> because this is for lossless, but for malicious files possibly.
> 

The addition uses unsigned types, so that overflow is defined.

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