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

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

[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);
+        int32_t i0 = i1 + *src2;
+        int32_t i2 = i1 + *src1;
         *src0++ = i0;
         *src1++ = i1;
         *src2++ = i2;
-- 
2.34.1

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

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

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

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

/Tomas

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

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

[Andreas Rheinhardt]

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

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

[Tomas Härdin]

tis 2022-09-27 klockan 13:20 +0200 skrev Andreas Rheinhardt:
> 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.

Wups, I was looking at the original code. You're right of course. What
about subtracting src0 (unsigned) from that result (signed)? Do they
get promoted to int64_t?


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

Tricky

/Tomas

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

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

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-09-27 klockan 13:20 +0200 skrev Andreas Rheinhardt:
>> 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.
> 
> Wups, I was looking at the original code. You're right of course. What
> about subtracting src0 (unsigned) from that result (signed)? Do they
> get promoted to int64_t?
> 

No. signed int + unsigned int uses unsigned.

> 
>> 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)).
> 
> Tricky
> 
> /Tomas
> 
> _______________________________________________
> 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".

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

[Andreas Rheinhardt]

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);
> +        int32_t i0 = i1 + *src2;
> +        int32_t i2 = i1 + *src1;
>          *src0++ = i0;
>          *src1++ = i1;
>          *src2++ = i2;

Will apply this patch tomorrow unless there are objections.

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