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 C19CD4862D for ; Wed, 10 Jul 2024 13:44:59 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 993C468DA73; Wed, 10 Jul 2024 16:44:56 +0300 (EEST) Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 5CC8468D9BD for ; Wed, 10 Jul 2024 16:44:49 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 5C80CC0008 for ; Wed, 10 Jul 2024 13:44:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1720619088; 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=7CByPrDJv2vgriU+iEbqbA75s82+0JTrgZmgJXXg9Vs=; b=cWQN+OxSbg7eFBctRuqcLsAxpe33UJTQlLn8VRiQshN7ZUye/O3/T4SWnLzvVy6c4mPBIn FBsEeiJLgCblzUv6YMK6Mq9ebOy1RhKcGUcKwFo+8Q38T0ZAGiIIQDB66vKcTDEkVHZ8Qx b0M6cP/2SHyg8qx/WU9o2ZaVC+cbczmBOx5kqdAZS5cdS31ZKNp9XFsNlBQnls0k9oxQeB Enyyb262uAD2reqonpl3SWqwbdI2u+/uZbN7f2hKXIxswpDBsS+H94tTDlb2l5ASeMkAWZ fqB+cVL+on901StdZbNAjZb1HKhLygCpCwq4brCvDGoIdw45m97vjmbCmqlbJA== Date: Wed, 10 Jul 2024 15:44:47 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240710134447.GH4991@pb2> References: <20240709113626.1836680-1-michael@niedermayer.cc> <172053107806.21847.11044848590089039731@lain.khirnov.net> <20240709132810.GA4991@pb2> <172053807774.21847.8430412564103918732@lain.khirnov.net> <20240709220032.GE4991@pb2> <172059983713.21847.3731174080920299257@lain.khirnov.net> MIME-Version: 1.0 In-Reply-To: <172059983713.21847.3731174080920299257@lain.khirnov.net> X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH] avutil/imgutils: av_image_check_size2() ensure width and height fit in 32bit 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="===============5736660897801648868==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============5736660897801648868== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Fv4aQf9orjE3TOVF" Content-Disposition: inline --Fv4aQf9orjE3TOVF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 10, 2024 at 10:23:57AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-07-10 00:00:32) > > On Tue, Jul 09, 2024 at 05:14:37PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2024-07-09 15:28:10) > > > > On Tue, Jul 09, 2024 at 03:17:58PM +0200, Anton Khirnov wrote: > > > > > > ensure width and height fit in 32bit > > > > >=20 > > > > > why? > > > >=20 > > > > because not everyone wants undefined behavior > > > > because not everyone wants security issues > > > > because we dont support width and height > 32bit and its easier to = check in a central place > > > > because the changed codes purpose is to check if the image paramate= rs are > > > > within what we support, and width of 100 billion is not. You ca= n try > > > > all encoders with 100billion width. Then try to decode. > > > > Iam curious, how many work, how many fail and how they fail > > > > how many invalid bitstreams with no warning, how many undefined= behaviors, ... > > > >=20 > > > > Simply building FFmpeg on a platform with 64bit ints doesnt update > > > > ISO and ITU standards to allow larger values > > >=20 > > > Quoting Michael Niedermayer (2020-10-07 16:45:56): > > > > At least in code i wrote and write i consider it a bug if it would > > > > assume sizeof(int/unsigned) =3D=3D 4 > > >=20 > > > Make up your mind. > >=20 > > Where do you see a contradiction ? > >=20 > > 2020: assuming sizeof(int/unsigned) =3D=3D 4 is a bug > > 2024: we do not support more than 32bit width and height, > > nor is that supported by the majority of codec bitsterams and for= mats > > -> We thus should in a central place check that instead of genera= ting > > undefined behavior and security issues > >=20 > > What i suggest IS actually fixing a "sizeof(int/unsigned) =3D=3D 4" bug > >=20 > > If someone wants to make the codebase work with 64bit width and height,= this > > should not be limited to "int is 64bit" systems that would be a very se= riously > > broken design and also very illogic. >=20 > I don't see any existing conditions on video dimensions being 32bit, the > condition currently is that every dimension and their product must fit > in an int. I also don't see what actual problems does this patch > address. >=20 > > Also your terse replies feel a bit rude >=20 > What a coincidence, your wall of patronizing blah blah that does nothing = to > actually answer my original question also seems pretty rude to me. Reap > what you sow. Lets take my reply and go over it: because not everyone wants undefined behavior because not everyone wants security issues because we dont support width and height > 32bit and its easier to chec= k in a central place When writing code, it has generally not been considered that width or heigh= t would exceed 32bit. int can be 64bit yes, but not width and height. So every line needs = to be reviewed, if you want to use it with 64bit width and height. Either way, the whole subject is mood as close to nothing supports this. movenc: avio_wb16(pb, track->par->width); /* Video width */ avio_wb16(pb, track->height); /* Video height */ mxfenc: avio_wb32(pb, stored_width); avio_wb32(pb, stored_height>>sc->interlaced); avienc: avio_wl16(pb, par->width); avio_wl16(pb, par->height); matroska: maybe nutenc: yes also lets assume either width and height is the smallest value over 32bit, = 0x80000000 what modern video codec supports this ? realistically you need at least a full row of macroblocks, and 3 frames, th= ats 96gb for a 16pixel high and 2 billion pixel long gray scale image (which is not = a realistic case, people encode images closer to square btw) a more normal 16:9 image would probably fit if you combine all memory =66rom all humans on earth. So i dont know but i think memory prices need t= o fall a bit before this becomes a real use cases and by then we will have int128_t like= ly making this cleaner and easier to support and again, like i said already, if we want to support 64bit width and height this has nothing to do with what int on a platform is. width and height should be changed to int64_t or a similar type through the= codebase IFF that is wanted Also there is code that will outright break, again please try this instead = of pretending it would work. for example all our code assumes product of 2 of width, height, a aspect ratio component fit in a int64_t this is violated with width or height being larger than 32bit Do you still object to a 32bit check on width and height ? If not i intend to apply a patch adding such limits If you object i will take this to the TC thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart than the original author, trying to rewrite it will not make it better. --Fv4aQf9orjE3TOVF Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZo6QRQAKCRBhHseHBAsP qwQ0AKCAoxhXeG+fsnYsEWOyPpY/cIqYCQCfXyYSX3BQLZnCeirw14xObYYKPm0= =7AFO -----END PGP SIGNATURE----- --Fv4aQf9orjE3TOVF-- --===============5736660897801648868== 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". --===============5736660897801648868==--