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

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

[Michael Niedermayer]

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);
     }
-- 
2.45.2

_______________________________________________
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".

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

[Anton Khirnov]

> ensure width and height fit in 32bit

why?

-- 
Anton Khirnov
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1058 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1414 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1990 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5016 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Paul B Mahol]

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".
>
_______________________________________________
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".

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

[Alexander Strasser via ffmpeg-devel]

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
_______________________________________________
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".

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

[Vittorio Giovara]

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
_______________________________________________
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".