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

[Tomas Härdin]

tis 2022-09-27 klockan 21:20 +0200 skrev Andreas Rheinhardt:
> 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.
> 
> Does this mean that this patch is ok?

Sure, it's better than before

/Tomas