| Message ID | 20240709113626.1836680-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avutil/imgutils: av_image_check_size2() ensure width and height fit in 32bit | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
> ensure width and height fit in 32bit
why?
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 thx [...]
On Tue, Jul 09, 2024 at 03:28:10PM +0200, Michael Niedermayer wrote: > 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 but theres more :) if we allow 64bit width and height, every check on the pixel number just broke because w*(uint64_t)h just doesnt work anymore. a 64bit int isnt giving us a int128_t. So many checks related to width and height suddenly become more fragile as theres not a simply larger type thx [...]
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.
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. Also your terse replies feel a bit rude thx [...]
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.
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 [...]
Quoting Michael Niedermayer (2024-07-10 15:44:47) > 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 In my first reply in this thread I asked for a very simple thing - justify this commit, as the commit message provided ZERO arguments for why is this done and what it actually improves. Instead of answering the question you keep painting ever wordier walls of text. I'm not reading all that. Just answer the question, in the form suitable for a commit message.
On Wed, Jul 10, 2024 at 12:00 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > 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. > > Also your terse replies feel a bit rude > One can not simply reason with toxic humanoids. > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No great genius has ever existed without some touch of madness. -- > Aristotle > _______________________________________________ > 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". >
On 2024-07-10 15:51 +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-07-10 15:44:47) > > 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 > > In my first reply in this thread I asked for a very simple thing - > justify this commit, as the commit message provided ZERO arguments for > why is this done and what it actually improves. > > Instead of answering the question you keep painting ever wordier walls > of text. I'm not reading all that. Just answer the question, in the form > suitable for a commit message. As most of the time a communications problem AFAICS :( The answer Anton was probably looking for: The intention of av_image_check_size2 (and it it's predecessors) always was to use a bigger integer type (64bit) to check width and height fit into the limits of the next smaller integer type (32bit). Unfortunately it wasn't spelled out explicitly in the commit message back then, and the implementation incorrectly assumed that INT_MAX would always refer to 32bit signed max. Alexander
On Tue, Jul 9, 2024 at 1:44 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > on "INT is 64bit" systems they may have been false > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavutil/imgutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c > index d2463815637..b738cff37c2 100644 > --- a/libavutil/imgutils.c > +++ b/libavutil/imgutils.c > @@ -298,7 +298,7 @@ int av_image_check_size2(unsigned int w, unsigned int > h, int64_t max_pixels, enu > stride = 8LL*w; > stride += 128*8; > > - if ((int)w<=0 || (int)h<=0 || stride >= INT_MAX || > stride*(uint64_t)(h+128) >= INT_MAX) { > + if (w==0 || h==0 || w > INT32_MAX || h > INT32_MAX || stride >= > INT_MAX || stride*(uint64_t)(h+128) >= INT_MAX) { > av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is > invalid\n", w, h); > return AVERROR(EINVAL); > } While changing this line, it would be nice to have an explanation on what the conditions do - for example why is stride multiplied by height+128? And why is stride checked against INT_MAX and w/h against INT32_MAX? Thanks Vittorio
diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c index d2463815637..b738cff37c2 100644 --- a/libavutil/imgutils.c +++ b/libavutil/imgutils.c @@ -298,7 +298,7 @@ int av_image_check_size2(unsigned int w, unsigned int h, int64_t max_pixels, enu stride = 8LL*w; stride += 128*8; - if ((int)w<=0 || (int)h<=0 || stride >= INT_MAX || stride*(uint64_t)(h+128) >= INT_MAX) { + if (w==0 || h==0 || w > INT32_MAX || h > INT32_MAX || stride >= INT_MAX || stride*(uint64_t)(h+128) >= INT_MAX) { av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h); return AVERROR(EINVAL); }
on "INT is 64bit" systems they may have been false Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavutil/imgutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)