| Message ID | GV1P250MB07377FF4AAC085EC65213D3B8F559@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 3b09e8f5e9c0f419bd8cef7f4dd839cb4ed528d6 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/jpeg2000dsp: Use unsigned to avoid overflow | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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
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
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".
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
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;
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(-)