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: 2713 bytes --]

On Mon, Apr 29, 2024 at 12:26:24AM +0200, Lynne wrote:
> Apr 28, 2024, 23:57 by michael@niedermayer.cc:
> 
> > 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.
> >
> 
> Could you instead use 64-bit arithmetic for `coef`?
> Such a general assert here could result in false alarms, like the AAC
> encoder suffered from before, which would make development harder.

If you dont like the assert, then its best to just drop the patch.
It doesnt fix anything, it was just intended to clarify that the
case of len < 4 doesnt occur. The coverity issue is a false positive

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

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