[FFmpeg-devel] [PATCH] avutil/imgutils: av_image_check_size2() ensure width and height fit in 32bit

[Michael Niedermayer]

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
> > > > > 
> > > > > why?
> > > > 
> > > > 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 paramaters are
> > > >     within what we support, and width of 100 billion is not. You can 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, ...
> > > > 
> > > > Simply building FFmpeg on a platform with 64bit ints doesnt update
> > > > ISO and ITU standards to allow larger values
> > > 
> > > 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) == 4
> > > 
> > > Make up your mind.
> > 
> > Where do you see a contradiction ?
> > 
> > 2020: assuming sizeof(int/unsigned) == 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 formats
> >       -> We thus should in a central place check that instead of generating
> >       undefined behavior and security issues
> > 
> > What i suggest IS actually fixing a "sizeof(int/unsigned) == 4" bug
> > 
> > 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 seriously
> > broken design and also very illogic.
> 
> 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.
> 
> > Also your terse replies feel a bit rude
> 
> 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 check in a central place

When writing code, it has generally not been considered that width or height 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, thats 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
from all humans on earth. So i dont know but i think memory prices need to fall a bit
before this becomes a real use cases and by then we will have int128_t likely
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

[...]
-- 
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240710/9623e847/attachment.sig>