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

[Tomas Härdin]

tis 2022-09-27 klockan 13:40 +0200 skrev 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

Yeah, I had the test uncover some interesting bugs on my end when
developing, that probably don't happen with real files. But malicious
files potentially triggering UB is something we shouldn't ignore

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

We don't need saturation here, only that the behavior is not undefined.
Wrapping around is fine. The only place where saturation is performed
is when converting decoded and idwt'd coefficients to actual pixel data
in write_frame_*()

It's possible that for sufficiently large 16-bit frames with enough
decomposition levels that "lossless" encoding is not actually lossless
unless the encoder uses 64-bit integers.

j2kenc supports RGB48, nreslevels=7, which can run into this problem at
resolutions as low as 255x255 I think.

/Tomas