[FFmpeg-devel] [PATCH v2] avcodec/jpeg2000: Fix undefined behaviour in left shift operations

[Pierre-Anthony Lemieux]

The following no longer return a UB warning after the patch:

./configure --extra-cflags='-fno-sanitize-recover=undefined'
--toolchain=gcc-usan
make fate-jpeg2000dec
make fate-j2k-dwt
make fate-vsynth{1,2,3, _lena}

Anything else should be included? If not, I will merge at week's end.

On Wed, Dec 4, 2024 at 10:39 PM Osamu Watanabe
<owatanab at es.takushoku-u.ac.jp> wrote:
>
> This patch fixes undefined behaviour error in left shift
> operations in jpeg2000dec.c, jpeg2000htdec.c, and
> libavcodec/tests/jpeg2000dwt.c.
>
> Signed-off-by: Osamu Watanabe <owatanab at es.takushoku-u.ac.jp>
> ---
>  libavcodec/jpeg2000dec.c       | 6 +++---
>  libavcodec/jpeg2000htdec.c     | 6 +++---
>  libavcodec/tests/jpeg2000dwt.c | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index c9d8b025b1..84eebfd1b2 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -1886,10 +1886,10 @@ static void decode_sigpass(Jpeg2000T1Context *t1, int width, int height,
>                      if (ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ff_jpeg2000_getsigctxno(t1->flags[(y+1) * t1->stride + x+1] & flags_mask, bandno))) {
>                          int xorbit, ctxno = ff_jpeg2000_getsgnctxno(t1->flags[(y+1) * t1->stride + x+1] & flags_mask, &xorbit);
>                          if (t1->mqc.raw) {
> -                            t1->data[(y) * t1->stride + x] |= ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno) << 31;
> +                            t1->data[(y) * t1->stride + x] |= (uint32_t)(ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno)) << 31;
>                              t1->data[(y) * t1->stride + x] |= mask;
>                          } else {
> -                            t1->data[(y) * t1->stride + x] |= (ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno) ^ xorbit) << 31;
> +                            t1->data[(y) * t1->stride + x] |= (uint32_t)(ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno) ^ xorbit) << 31;
>                              t1->data[(y) * t1->stride + x] |= mask;
>                          }
>                          ff_jpeg2000_set_significance(t1, x, y,
> @@ -1969,7 +1969,7 @@ static void decode_clnpass(const Jpeg2000DecoderContext *s, Jpeg2000T1Context *t
>                      int xorbit;
>                      int ctxno = ff_jpeg2000_getsgnctxno(t1->flags[(y + 1) * t1->stride + x + 1] & flags_mask,
>                                                          &xorbit);
> -                    t1->data[(y) * t1->stride + x] |= (ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno) ^ xorbit) << 31;
> +                    t1->data[(y) * t1->stride + x] |= (uint32_t)(ff_mqc_decode(&t1->mqc, t1->mqc.cx_states + ctxno) ^ xorbit) << 31;
>                      t1->data[(y) * t1->stride + x] |= mask;
>                      ff_jpeg2000_set_significance(t1, x, y, t1->data[(y) * t1->stride + x] & INT32_MIN);
>                  }
> diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c
> index 186a6873ac..08140e06a9 100644
> --- a/libavcodec/jpeg2000htdec.c
> +++ b/libavcodec/jpeg2000htdec.c
> @@ -1070,7 +1070,7 @@ static void jpeg2000_process_stripes_block(StateVars *sig_prop, int i_s, int j_s
>              uint8_t *state_p = block_states + (i + 1) * stride + (j + 1);
>              if ((state_p[0] >> HT_SHIFT_REF) & 1) {
>                  bit = jpeg2000_peek_bit(sig_prop, magref_segment, magref_length);
> -                *sp |= (int32_t)bit << 31;
> +                *sp |= (uint32_t)bit << 31;
>              }
>          }
>      }
> @@ -1160,7 +1160,7 @@ jpeg2000_decode_magref_segment( uint16_t width, uint16_t block_height, const int
>                      jpeg2000_modify_state(i, j, stride, 1 << HT_SHIFT_REF_IND, block_states);
>                      bit = jpeg2000_import_magref_bit(&mag_ref, magref_segment, magref_length);
>                      tmp = 0xFFFFFFFE | (uint32_t)bit;
> -                    tmp <<= pLSB;
> +                    tmp = (uint32_t)tmp << pLSB;
>                      sp[0] &= tmp;
>                      sp[0] |= 1 << (pLSB - 1); // Add 0.5 (reconstruction parameter = 1/2)
>                  }
> @@ -1176,7 +1176,7 @@ jpeg2000_decode_magref_segment( uint16_t width, uint16_t block_height, const int
>                  jpeg2000_modify_state(i, j, stride, 1 << HT_SHIFT_REF_IND, block_states);
>                  bit = jpeg2000_import_magref_bit(&mag_ref, magref_segment, magref_length);
>                  tmp = 0xFFFFFFFE | (uint32_t)bit;
> -                tmp <<= pLSB;
> +                tmp = (uint32_t)tmp << pLSB;
>                  sp[0] &= tmp;
>                  sp[0] |= 1 << (pLSB - 1); // Add 0.5 (reconstruction parameter = 1/2)
>              }
> diff --git a/libavcodec/tests/jpeg2000dwt.c b/libavcodec/tests/jpeg2000dwt.c
> index 9b26440ca8..f9adb60564 100644
> --- a/libavcodec/tests/jpeg2000dwt.c
> +++ b/libavcodec/tests/jpeg2000dwt.c
> @@ -49,7 +49,7 @@ static int test_dwt(int *array, int *ref, int border[2][2], int decomp_levels, i
>      if (type == FF_DWT97_INT) {
>          // pre-scaling to simulate dequantization which places the binary point at 1 bit above from LSB
>          for (j = 0; j< s->linelen[decomp_levels-1][0] * s->linelen[decomp_levels-1][1]; j++)
> -            array[j] <<= I_PRESHIFT;
> +            array[j] = (uint32_t)array[j] << I_PRESHIFT;
>      }
>      ret = ff_dwt_decode(s, array);
>      if (ret < 0) {
> --
> 2.43.0
>