Re: [FFmpeg-devel] [PATCH 2/5] avcodec/aaccoder: assert that escape case len is not causing issues

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2084 bytes --]

On Sun, Apr 28, 2024 at 11:41:20PM +0200, Lynne wrote:
> Apr 28, 2024, 23:31 by michael@niedermayer.cc:
> 
> > Inspired by CID1465483 Unintentional integer overflow
> >
> > Sponsored-by: Sovereign Tech Fund
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/aaccoder.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c
> > index 4ce54ca8867..6e5817e237b 100644
> > --- a/libavcodec/aaccoder.c
> > +++ b/libavcodec/aaccoder.c
> > @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template(
> >  int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13);
> >  int len = av_log2(coef);
> >  
> > +                        av_assert2(len >= 4);
> > +
> >  put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2);
> >  put_sbits(pb, len, coef);
> >  }
> >
> 
> I'm not sure that's correct to do. Any specific cases where this happens?

if len is 3 or less then put_bits will have a negative value or
undefined shift

coverity sasid this:
" overflow_before_widen: Potentially overflowing expression 1 << len - 4 + 1 with type int (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type BitBuf (64 bits, unsigned). To avoid overflow, cast 1 to type BitBuf."

So what coverity really said is that the expression could exeed 32bit _because_ its
used in 64bit context. Thats just stupid from coverity also teh clip above
limits this to 13 bit so i dont see how it can overflow in the "too large" direction
and i marked this one as false positive.

I wasnt 100% sure about the too small side, i tested it and its never too small
but coverity didnt claim it could be too small, so that question is outside the issue
So i added a assert in this patch.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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