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 E029343534 for ; Thu, 16 Jun 2022 11:19:14 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 34EB368B7F3; Thu, 16 Jun 2022 14:19:11 +0300 (EEST) Received: from shout02.mail.de (shout02.mail.de [62.201.172.25]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 359A668B7D4 for ; Thu, 16 Jun 2022 14:19:05 +0300 (EEST) Received: from postfix03.mail.de (postfix03.bt.mail.de [10.0.121.127]) by shout02.mail.de (Postfix) with ESMTP id 8C658A0D1F for ; Thu, 16 Jun 2022 13:19:04 +0200 (CEST) Received: from smtp02.mail.de (smtp02.bt.mail.de [10.0.121.212]) by postfix03.mail.de (Postfix) with ESMTP id 7416980299 for ; Thu, 16 Jun 2022 13:19:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=mail.de; s=mailde202009; t=1655378344; bh=2iYqDDe5w73xXJ0BwlmGRzbILFj8y3RvSjE+gXl9EM0=; h=Message-ID:Date:Subject:To:From:From:To:CC:Subject:Reply-To; b=oaLSB+ebJ9OHEviwtP/nVxqYuGSsgzrkzron2ieaxToVnujJldUEnQZLktCc2AVG3 Jcl5v8dKYPSDs+hu4xy2/Vo338VCpI4WhrFJHsKay3zMXQp7n2IeQK7v3Pcxeyxwvn GQWyaI9QsL/hOV62VNluZmLRDkVILa0yFAMO4IRDgLHMEv/+IQ3YzoN1WshrWq2kHJ StFQ2frv1PnVWnJ0eae9ZIkGXJd4B842fbNGDPrZvcGTs51FJmMH+bLDejDrcPK/JS KujZLgrh/HaY6CyjAthMwUqS4Cph3hn8XE0r3PGAmoz/FHAuftSRETT3eTm73R1ZlP 8Bz9at7UvcxxQ== Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by smtp02.mail.de (Postfix) with ESMTPSA id 3E918A02DF for ; Thu, 16 Jun 2022 13:19:04 +0200 (CEST) Content-Type: multipart/mixed; boundary="------------TXwMh7gjXKO02Oc2qE0qNpx8" Message-ID: <141157fa-3768-a187-81a7-52b89aa10787@mail.de> Date: Thu, 16 Jun 2022 13:19:03 +0200 MIME-Version: 1.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <80816607-3610-9133-9d5d-7fee3040de06@mail.de> <82d9a6ac-5365-dae4-9825-eabec6b2151f@mail.de> From: Thilo Borgmann In-Reply-To: X-purgate: clean X-purgate: This mail is considered clean (visit http://www.eleven.de for further information) X-purgate-type: clean X-purgate-Ad: Categorized by eleven eXpurgate (R) http://www.eleven.de X-purgate: This mail is considered clean (visit http://www.eleven.de for further information) X-purgate: clean X-purgate-size: 4362 X-purgate-ID: 154282::1655378344-0000737C-64AE95C8/0/0 Subject: Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: This is a multi-part message in MIME format. --------------TXwMh7gjXKO02Oc2qE0qNpx8 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Am 16.06.22 um 12:40 schrieb Andreas Rheinhardt: > Thilo Borgmann: >> Am 16.06.22 um 12:13 schrieb Andreas Rheinhardt: >>> Thilo Borgmann: >>>> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c >>>> index a87562c8a3..833ce9e705 100644 >>>> --- a/libavcodec/dovi_rpu.c >>>> +++ b/libavcodec/dovi_rpu.c >>>> @@ -153,7 +153,7 @@ static inline uint64_t get_ue_coef(GetBitContext >>>> *gb, const AVDOVIRpuDataHeader >>>>       case RPU_COEFF_FIXED: >>>>           ipart = get_ue_golomb_long(gb); >>>>           fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom); >>>> -        return (ipart << hdr->coef_log2_denom) + fpart.u32; >>>> +        return ipart * (1 << hdr->coef_log2_denom) + fpart.u32; >>>>         case RPU_COEFF_FLOAT: >>>>           fpart.u32 = get_bits_long(gb, 32); >>>> @@ -172,7 +172,7 @@ static inline int64_t get_se_coef(GetBitContext >>>> *gb, const AVDOVIRpuDataHeader * >>>>       case RPU_COEFF_FIXED: >>>>           ipart = get_se_golomb_long(gb); >>>>           fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom); >>>> -        return (ipart << hdr->coef_log2_denom) + fpart.u32; >>>> +        return ipart * (1 << hdr->coef_log2_denom) + fpart.u32; >>>>         case RPU_COEFF_FLOAT: >>>>           fpart.u32 = get_bits_long(gb, 32); >>> >>> coef_log2_denom can be in the range 13..32. This means that 1 << >>> hdr->coef_log2_denom can be UB (namely if coef_log2_denom is 31 or 32 >>> for ordinary 32 bit ints); this time it is not UB that happens to work >>> as expected, because 1 << 32 will be 0 or 1 (depending upon the system) >>> and not 2^32. In case of get_ue_coef() this actually adds UB to >>> otherwise fine code. >> >> So 1LL it needs to be, not? Am I still missing something? >> > > This version should not add new UB. > I btw don't get why you are changing get_ue_coef() at all: It's fine > as-is; consistency should only trump when the choice is between several > equally good alternatives which is IMO not true here. (The reason that C > makes left shifts of negative values UB is because of non-two's > complement systems, so it is unfortunate for a project like FFmpeg that > requires two's complement that it has to workaround this restriction by > using a * (1 << b).) Just overthinking consistency, I guess. v4 changes get_se_coef() only. Thanks, Thilo --------------TXwMh7gjXKO02Oc2qE0qNpx8 Content-Type: text/plain; charset=UTF-8; name="v4-0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-n.patch" Content-Disposition: attachment; filename*0="v4-0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-n.pa"; filename*1="tch" Content-Transfer-Encoding: base64 RnJvbSA4NTQyOWJlMTdiZjIzODY0MmFlYWNmOTNlNGQ3YzE2ZDRjNGIwM2EzIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBNaWNoYWVsIEdvdWxldCA8bWdvdWxldEBmYi5jb20+ CkRhdGU6IFRodSwgMTYgSnVuIDIwMjIgMTM6MTc6MjUgKzAyMDAKU3ViamVjdDogW1BBVENI IHY0XSBsYXZjL2RvdmlfcnB1OiBGaXggVUIgZm9yIHBvc3NpYmxlIGxlZnQgc2hpZnQgb2Yg bmVnYXRpdmUKIHZhbHVlcwoKSXQgaXMgdW5kZWZpbmVkIHRvIGxlZnQtc2hpZnQgYSBuZWdh dGl2ZSB2YWx1ZS4KLS0tCiBsaWJhdmNvZGVjL2RvdmlfcnB1LmMgfCAyICstCiAxIGZpbGUg Y2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkKCmRpZmYgLS1naXQgYS9s aWJhdmNvZGVjL2RvdmlfcnB1LmMgYi9saWJhdmNvZGVjL2RvdmlfcnB1LmMKaW5kZXggYTg3 NTYyYzhhMy4uZGQzODkzNjU1MiAxMDA2NDQKLS0tIGEvbGliYXZjb2RlYy9kb3ZpX3JwdS5j CisrKyBiL2xpYmF2Y29kZWMvZG92aV9ycHUuYwpAQCAtMTcyLDcgKzE3Miw3IEBAIHN0YXRp YyBpbmxpbmUgaW50NjRfdCBnZXRfc2VfY29lZihHZXRCaXRDb250ZXh0ICpnYiwgY29uc3Qg QVZET1ZJUnB1RGF0YUhlYWRlciAqCiAgICAgY2FzZSBSUFVfQ09FRkZfRklYRUQ6CiAgICAg ICAgIGlwYXJ0ID0gZ2V0X3NlX2dvbG9tYl9sb25nKGdiKTsKICAgICAgICAgZnBhcnQudTMy ID0gZ2V0X2JpdHNfbG9uZyhnYiwgaGRyLT5jb2VmX2xvZzJfZGVub20pOwotICAgICAgICBy ZXR1cm4gKGlwYXJ0IDw8IGhkci0+Y29lZl9sb2cyX2Rlbm9tKSArIGZwYXJ0LnUzMjsKKyAg ICAgICAgcmV0dXJuIGlwYXJ0ICogKDFMTCA8PCBoZHItPmNvZWZfbG9nMl9kZW5vbSkgKyBm cGFydC51MzI7CiAKICAgICBjYXNlIFJQVV9DT0VGRl9GTE9BVDoKICAgICAgICAgZnBhcnQu dTMyID0gZ2V0X2JpdHNfbG9uZyhnYiwgMzIpOwotLSAKMi4yMC4xIChBcHBsZSBHaXQtMTE3 KQoK --------------TXwMh7gjXKO02Oc2qE0qNpx8 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". --------------TXwMh7gjXKO02Oc2qE0qNpx8--