[FFmpeg-devel] [PATCH] avcodec/jpeg2000dwt: Fix left shift of negative number

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt:
>> Fixes the j2k-dwt FATE-test; also fixes #9945.
>> (I don't know whether the multiplication can overflow.)
> 
> The 5/3 transform is used in lossless mode and therefore shouldn't
> overflow for normal use cases. But someone can of course craft a
> malicious file
> 
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavcodec/jpeg2000dwt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c
>> index f2da7307c4..34e33553f7 100644
>> --- a/libavcodec/jpeg2000dwt.c
>> +++ b/libavcodec/jpeg2000dwt.c
>> @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1)
>>  
>>      if (i1 <= i0 + 1) {
>>          if (i0 == 1)
>> -            p[1] <<= 1;
>> +            p[1] *= 2;
> 
> To trigger an actual overflow here you need enough coefficient bits and
> enough decomposition levels, meaning also huge resolution. Resolution
> is capped at what 32k x 32k currently? That means you need 17-bit
> coefficients at the lowest levels to get over INT_MAX. I'm not actually
> sure what the limits for that in jpeg2000 is, but 12-bit lossless would
> certaily hit these levels at 5 or more decomp levels. I have samples
> that use 6, and it's easy to generate ones that have even more.
> 

FYI: This is not triggered by an actual jpeg2000 sample (not even a
malicious one), this is triggered by the jpeg2000dwt test tool
(libavcodec/tests/jpeg2000dwt.c). It might entirely be possible that the
tool tests something that can't happen during decoding (and not only
during decoding of spec-compliant files).

> To be really safe we'd need to use something like
> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
> and maybe define fallbacks for other compilers.
> 

Take a look at av_sat_add64_c() and similar functions.

> This is part of why I've been harping about formal verification.
>