From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 315C24A3DD for ; Mon, 29 Apr 2024 22:58:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6E55368D597; Tue, 30 Apr 2024 01:58:48 +0300 (EEST) Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id BC78C68D53D for ; Tue, 30 Apr 2024 01:58:41 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id EEF3820007 for ; Mon, 29 Apr 2024 22:58:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1714431521; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hPtuYidPuec1jl3jNCXwb05v7Uo3aZWOH6j04FAu1Xw=; b=MxDKjvg8TL9f98zj0y6DT74xr6j9aYsEhc0D3pS3X5hHnPgI2M2mSoKNhVMShOJ7CW/Osd xz6I1kWwpWHZtfODaNJAkNqJxHPPt2WxeMDVB1sBkoeRKDQL7vcK4Rd4aIDkqxZw7N8LP8 NzH0I/caaDmBeq//XWFtNWhwe9xjs7IIqlTZRXRfRx7WKq1yBmXRpocCD0xKszQaJBMm3R /1UUMfcUQGiiGCXTTZ2T2K9UxgpMWzBoP5Aq4y0e+vSYEEfuIQSSbYkbrheqqByqmrixEP zXbzj1DFBB64eqZMyPJyuOphC79QEFHd/SUfnBwuknohazKcvwQitTIZs4kgZg== Date: Tue, 30 Apr 2024 00:58:40 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240429225840.GM6420@pb2> References: <20240428213052.3800493-1-michael@niedermayer.cc> <20240428213052.3800493-2-michael@niedermayer.cc> <20240428215725.GK6420@pb2> MIME-Version: 1.0 In-Reply-To: X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 2/5] avcodec/aaccoder: assert that escape case len is not causing issues X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============5561206352027217092==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============5561206352027217092== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Xuy9ZWN0QT3SlSng" Content-Disposition: inline --Xuy9ZWN0QT3SlSng Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 29, 2024 at 12:26:24AM +0200, Lynne wrote: > Apr 28, 2024, 23:57 by michael@niedermayer.cc: >=20 > > 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 > >> > --- > >> > 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_encod= e_band_cost_template( > >> > int coef =3D av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); > >> > int len =3D av_log2(coef); > >> >=20 > >> > + av_assert2(len >=3D 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 happe= ns? > >> > > > > 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 b= its, 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 ab= ove > > limits this to 13 bit so i dont see how it can overflow in the "too lar= ge" direction > > and i marked this one as false positive. > > > > I wasnt 100% sure about the too small side, i tested it and its never t= oo small > > but coverity didnt claim it could be too small, so that question is out= side the issue > > So i added a assert in this patch. > > >=20 > 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 [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu --Xuy9ZWN0QT3SlSng Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZjAmFgAKCRBhHseHBAsP q/LqAKCLQ1PNYpDOI5IL28Qb/QihAO1MKQCeLshyd1Bpp6p8XTHEsc80Z5ipCqY= =6bhw -----END PGP SIGNATURE----- --Xuy9ZWN0QT3SlSng-- --===============5561206352027217092== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============5561206352027217092==--