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

From: Lynne <dev@lynne.ee>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 2/5] avcodec/aaccoder: assert that escape case len is not causing issues
Date: Mon, 29 Apr 2024 00:26:24 +0200 (CEST)
Message-ID: <NwaoFbA--3-9@lynne.ee> (raw)
In-Reply-To: <20240428215725.GK6420@pb2>

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