[FFmpeg-devel] opt: check image size when setting it

Submitted by Andreas Cadhalpun on Dec. 10, 2016, 3:11 p.m.

Details

Message ID 3992956b-2511-fcbf-51f0-7f4627b55f0f@googlemail.com
State New
Headers show

Commit Message

Andreas Cadhalpun Dec. 10, 2016, 3:11 p.m.
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavutil/opt.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Dec. 10, 2016, 3:19 p.m.
2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavutil/opt.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index f855ccb..f713d3f 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -32,6 +32,7 @@
>  #include "common.h"
>  #include "dict.h"
>  #include "eval.h"
> +#include "imgutils.h"
>  #include "log.h"
>  #include "parseutils.h"
>  #include "pixdesc.h"
> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>          return 0;
>      }
>      ret = av_parse_video_size(dst, dst + 1, val);
> -    if (ret < 0)
> +    if (ret < 0) {
>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
> +        return ret;
> +    }
> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
> +    if (ret < 0) {
> +        *dst = 0;
> +        *(dst + 1) = 0;
> +    }

Doesn't this break valid usecases?

Carl Eugen
wm4 Dec. 10, 2016, 3:26 p.m.
On Sat, 10 Dec 2016 16:11:15 +0100
Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavutil/opt.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index f855ccb..f713d3f 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -32,6 +32,7 @@
>  #include "common.h"
>  #include "dict.h"
>  #include "eval.h"
> +#include "imgutils.h"
>  #include "log.h"
>  #include "parseutils.h"
>  #include "pixdesc.h"
> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>          return 0;
>      }
>      ret = av_parse_video_size(dst, dst + 1, val);
> -    if (ret < 0)
> +    if (ret < 0) {
>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
> +        return ret;
> +    }
> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
> +    if (ret < 0) {
> +        *dst = 0;
> +        *(dst + 1) = 0;
> +    }
>      return ret;
>  }
>  

I'd argue that rather than doing this, image allocation functions etc.
should error out if the dimensions are too large.

This way the allowed dimensions could be enlarged (e.g. by taking the
pixel format into account) without changing everything from int to
size_t.
Hendrik Leppkes Dec. 10, 2016, 3:54 p.m.
On Sat, Dec 10, 2016 at 4:26 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Sat, 10 Dec 2016 16:11:15 +0100
> Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavutil/opt.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>>          return 0;
>>      }
>>      ret = av_parse_video_size(dst, dst + 1, val);
>> -    if (ret < 0)
>> +    if (ret < 0) {
>>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
>> +        return ret;
>> +    }
>> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +    if (ret < 0) {
>> +        *dst = 0;
>> +        *(dst + 1) = 0;
>> +    }
>>      return ret;
>>  }
>>
>
> I'd argue that rather than doing this, image allocation functions etc.
> should error out if the dimensions are too large.
>
> This way the allowed dimensions could be enlarged (e.g. by taking the
> pixel format into account) without changing everything from int to
> size_t.

I agree, av_image_check_size has some arbitrary technical limits in it
right now that may not necessarily apply everywhere you want to set an
image size.
Allocations would need to be checked anyway with or without this check
because most options accessible through AVOptions are also accessible
through other means, so I would rather leave the decision of the
maximum size up to the code that uses these values.

- Hendrik
Andreas Cadhalpun Dec. 10, 2016, 3:55 p.m.
On 10.12.2016 16:19, Carl Eugen Hoyos wrote:
> 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavutil/opt.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>>          return 0;
>>      }
>>      ret = av_parse_video_size(dst, dst + 1, val);
>> -    if (ret < 0)
>> +    if (ret < 0) {
>>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
>> +        return ret;
>> +    }
>> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +    if (ret < 0) {
>> +        *dst = 0;
>> +        *(dst + 1) = 0;
>> +    }
> 
> Doesn't this break valid usecases?

None that I'm aware of. Which usecases were you thinking about?

Best regards,
Andreas
Andreas Cadhalpun Dec. 10, 2016, 3:55 p.m.
On 10.12.2016 16:26, wm4 wrote:
> On Sat, 10 Dec 2016 16:11:15 +0100
> Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavutil/opt.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>>          return 0;
>>      }
>>      ret = av_parse_video_size(dst, dst + 1, val);
>> -    if (ret < 0)
>> +    if (ret < 0) {
>>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
>> +        return ret;
>> +    }
>> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +    if (ret < 0) {
>> +        *dst = 0;
>> +        *(dst + 1) = 0;
>> +    }
>>      return ret;
>>  }
>>  
> 
> I'd argue that rather than doing this, image allocation functions etc.
> should error out if the dimensions are too large.

That can be done in addition to this.

> This way the allowed dimensions could be enlarged (e.g. by taking the
> pixel format into account) without changing everything from int to
> size_t.

If that is done, the hard limit in av_image_check_size should check for
the maximum allowed value of any pixel format.
But a check here is needed to make sure that width * height doesn't overflow.

Best regards,
Andreas
Carl Eugen Hoyos Dec. 10, 2016, 3:57 p.m.
2016-12-10 16:54 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:

> Allocations would need to be checked anyway with or without this check

Do you think this is related?

Carl Eugen
Carl Eugen Hoyos Dec. 10, 2016, 4 p.m.
2016-12-10 16:55 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> On 10.12.2016 16:19, Carl Eugen Hoyos wrote:
>> 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>> ---
>>>  libavutil/opt.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index f855ccb..f713d3f 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -32,6 +32,7 @@
>>>  #include "common.h"
>>>  #include "dict.h"
>>>  #include "eval.h"
>>> +#include "imgutils.h"
>>>  #include "log.h"
>>>  #include "parseutils.h"
>>>  #include "pixdesc.h"
>>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>>>          return 0;
>>>      }
>>>      ret = av_parse_video_size(dst, dst + 1, val);
>>> -    if (ret < 0)
>>> +    if (ret < 0) {
>>>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
>>> +        return ret;
>>> +    }
>>> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>>> +    if (ret < 0) {
>>> +        *dst = 0;
>>> +        *(dst + 1) = 0;
>>> +    }
>>
>> Doesn't this break valid usecases?
>
> None that I'm aware of. Which usecases were you thinking about?

I thought that if this check is more strict than it was, there must
be a usecase that breaks but I wasn't immediately successful
in finding one.

Carl Eugen
Hendrik Leppkes Dec. 10, 2016, 4:29 p.m.
On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 10.12.2016 16:26, wm4 wrote:
>> On Sat, 10 Dec 2016 16:11:15 +0100
>> Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
>>
>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>> ---
>>>  libavutil/opt.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index f855ccb..f713d3f 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -32,6 +32,7 @@
>>>  #include "common.h"
>>>  #include "dict.h"
>>>  #include "eval.h"
>>> +#include "imgutils.h"
>>>  #include "log.h"
>>>  #include "parseutils.h"
>>>  #include "pixdesc.h"
>>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>>>          return 0;
>>>      }
>>>      ret = av_parse_video_size(dst, dst + 1, val);
>>> -    if (ret < 0)
>>> +    if (ret < 0) {
>>>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
>>> +        return ret;
>>> +    }
>>> +    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>>> +    if (ret < 0) {
>>> +        *dst = 0;
>>> +        *(dst + 1) = 0;
>>> +    }
>>>      return ret;
>>>  }
>>>
>>
>> I'd argue that rather than doing this, image allocation functions etc.
>> should error out if the dimensions are too large.
>
> That can be done in addition to this.
>
>> This way the allowed dimensions could be enlarged (e.g. by taking the
>> pixel format into account) without changing everything from int to
>> size_t.
>
> If that is done, the hard limit in av_image_check_size should check for
> the maximum allowed value of any pixel format.
> But a check here is needed to make sure that width * height doesn't overflow.

Why is that needed? Also, overflow what range exactly? int64? The
generic option code should not make any assumptions how the data is
going to be used. As long as its not multiplied right here and now,
there is no reason to check.
As said in an earlier mail, the check doesn't negate the need to check
in other places, because AVOption is only one way to set values, so I
would rather prefer avoiding adding more artificial limits in very
generic code.

- Hendrik
Andreas Cadhalpun Dec. 10, 2016, 5:20 p.m.
On 10.12.2016 17:29, Hendrik Leppkes wrote:
> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> If that is done, the hard limit in av_image_check_size should check for
>> the maximum allowed value of any pixel format.
>> But a check here is needed to make sure that width * height doesn't overflow.
> 
> Why is that needed?

Because the framework currently doesn't support larger sizes and setting
options to invalid values doesn't make much sense.

> Also, overflow what range exactly? int64?

The range of valid image dimension, which is what av_image_check_size
is documented to check.

> The generic option code should not make any assumptions how the data is
> going to be used. As long as its not multiplied right here and now,
> there is no reason to check.

It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
is used as image size, so it shouldn't accept values that are invalid
dimensions in our framework. Also it already doesn't accept negative
values. Would you prefer to remove this check?

> As said in an earlier mail, the check doesn't negate the need to check
> in other places, because AVOption is only one way to set values, so I
> would rather prefer avoiding adding more artificial limits in very
> generic code.

The alternative is that every program setting the image size needs to
check if it is valid before setting the option. That's quite inconvenient.

Best regards,
Andreas
Hendrik Leppkes Dec. 10, 2016, 5:40 p.m.
On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>> <andreas.cadhalpun@googlemail.com> wrote:
>>> If that is done, the hard limit in av_image_check_size should check for
>>> the maximum allowed value of any pixel format.
>>> But a check here is needed to make sure that width * height doesn't overflow.
>>
>> Why is that needed?
>
> Because the framework currently doesn't support larger sizes and setting
> options to invalid values doesn't make much sense.
>
>> Also, overflow what range exactly? int64?
>
> The range of valid image dimension, which is what av_image_check_size
> is documented to check.
>
>> The generic option code should not make any assumptions how the data is
>> going to be used. As long as its not multiplied right here and now,
>> there is no reason to check.
>
> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
> is used as image size, so it shouldn't accept values that are invalid
> dimensions in our framework. Also it already doesn't accept negative
> values. Would you prefer to remove this check?

Negative size is inherently invalid for image sizes, and not an
arbitrary limit, so that argument makes no sense.

>
>> As said in an earlier mail, the check doesn't negate the need to check
>> in other places, because AVOption is only one way to set values, so I
>> would rather prefer avoiding adding more artificial limits in very
>> generic code.
>
> The alternative is that every program setting the image size needs to
> check if it is valid before setting the option. That's quite inconvenient.

No, the point is that everything that _uses_ these values needs to
check it either way, so adding checks here doesn't seem to improve
anything and just adds extra burden when these limits are
changed/improved (say by making them pixfmt aware as discussed in
another thread, which this function could never know).

What exactly is this check supposed to fix/improve? Since all
libraries need to check it anyway and would error then, littering
earlier code paths with extra checks seems to not help much.

- Hendrik
Andreas Cadhalpun Dec. 10, 2016, 5:53 p.m.
On 10.12.2016 18:40, Hendrik Leppkes wrote:
> On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>>> <andreas.cadhalpun@googlemail.com> wrote:
>>>> If that is done, the hard limit in av_image_check_size should check for
>>>> the maximum allowed value of any pixel format.
>>>> But a check here is needed to make sure that width * height doesn't overflow.
>>>
>>> Why is that needed?
>>
>> Because the framework currently doesn't support larger sizes and setting
>> options to invalid values doesn't make much sense.
>>
>>> Also, overflow what range exactly? int64?
>>
>> The range of valid image dimension, which is what av_image_check_size
>> is documented to check.
>>
>>> The generic option code should not make any assumptions how the data is
>>> going to be used. As long as its not multiplied right here and now,
>>> there is no reason to check.
>>
>> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
>> is used as image size, so it shouldn't accept values that are invalid
>> dimensions in our framework. Also it already doesn't accept negative
>> values. Would you prefer to remove this check?
> 
> Negative size is inherently invalid for image sizes, and not an
> arbitrary limit, so that argument makes no sense.

However, some file formats encode image sizes as negative values to
give them a special meaning like reversed image buffer. So it's not
entirely hypothetical to set height to a negative value.
(The current ffmpeg code does this and only later uses FFABS.)

>>> As said in an earlier mail, the check doesn't negate the need to check
>>> in other places, because AVOption is only one way to set values, so I
>>> would rather prefer avoiding adding more artificial limits in very
>>> generic code.
>>
>> The alternative is that every program setting the image size needs to
>> check if it is valid before setting the option. That's quite inconvenient.
> 
> No, the point is that everything that _uses_ these values needs to
> check it either way, so adding checks here doesn't seem to improve
> anything

The improvement is that width/height are at no point set to invalid values.

> and just adds extra burden when these limits are
> changed/improved (say by making them pixfmt aware as discussed in
> another thread, which this function could never know).

There is no extra burden, because av_image_check_size would have to
be changed in that case anyway to accept the largest value valid
for any pixel format.

> What exactly is this check supposed to fix/improve? Since all
> libraries need to check it anyway and would error then, littering
> earlier code paths with extra checks seems to not help much.

In general, values should be checked for validity when they are set.

Best regards,
Andreas
Hendrik Leppkes Dec. 11, 2016, 9:04 a.m.
On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 10.12.2016 18:40, Hendrik Leppkes wrote:
>> On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
>> <andreas.cadhalpun@googlemail.com> wrote:
>>> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>>>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>>>> <andreas.cadhalpun@googlemail.com> wrote:
>>>>> If that is done, the hard limit in av_image_check_size should check for
>>>>> the maximum allowed value of any pixel format.
>>>>> But a check here is needed to make sure that width * height doesn't overflow.
>>>>
>>>> Why is that needed?
>>>
>>> Because the framework currently doesn't support larger sizes and setting
>>> options to invalid values doesn't make much sense.
>>>
>>>> Also, overflow what range exactly? int64?
>>>
>>> The range of valid image dimension, which is what av_image_check_size
>>> is documented to check.
>>>
>>>> The generic option code should not make any assumptions how the data is
>>>> going to be used. As long as its not multiplied right here and now,
>>>> there is no reason to check.
>>>
>>> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
>>> is used as image size, so it shouldn't accept values that are invalid
>>> dimensions in our framework. Also it already doesn't accept negative
>>> values. Would you prefer to remove this check?
>>
>> Negative size is inherently invalid for image sizes, and not an
>> arbitrary limit, so that argument makes no sense.
>
> However, some file formats encode image sizes as negative values to
> give them a special meaning like reversed image buffer. So it's not
> entirely hypothetical to set height to a negative value.
> (The current ffmpeg code does this and only later uses FFABS.)
>
>>>> As said in an earlier mail, the check doesn't negate the need to check
>>>> in other places, because AVOption is only one way to set values, so I
>>>> would rather prefer avoiding adding more artificial limits in very
>>>> generic code.
>>>
>>> The alternative is that every program setting the image size needs to
>>> check if it is valid before setting the option. That's quite inconvenient.
>>
>> No, the point is that everything that _uses_ these values needs to
>> check it either way, so adding checks here doesn't seem to improve
>> anything
>
> The improvement is that width/height are at no point set to invalid values.
>
>> and just adds extra burden when these limits are
>> changed/improved (say by making them pixfmt aware as discussed in
>> another thread, which this function could never know).
>
> There is no extra burden, because av_image_check_size would have to
> be changed in that case anyway to accept the largest value valid
> for any pixel format.
>

av_image_check_size2 was introduced by Michael now which works in the
context of a pix_fmt, which for many formats allows significantly
larger images then the old function (up to factor 4 larger)
I still see the problem that this option code does not know which
pix_fmt the numbers relate to and as such would keep the old limit in
place despite there being no technical reasons for it.

- Hendrik
Andreas Cadhalpun Dec. 11, 2016, 6:48 p.m.
On 11.12.2016 10:04, Hendrik Leppkes wrote:
> On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> On 10.12.2016 18:40, Hendrik Leppkes wrote:
>>> and just adds extra burden when these limits are
>>> changed/improved (say by making them pixfmt aware as discussed in
>>> another thread, which this function could never know).
>>
>> There is no extra burden, because av_image_check_size would have to
>> be changed in that case anyway to accept the largest value valid
>> for any pixel format.
>>
> 
> av_image_check_size2 was introduced by Michael now which works in the
> context of a pix_fmt, which for many formats allows significantly
> larger images then the old function (up to factor 4 larger)

Actually there is a factor of 64 between AV_PIX_FMT_MONOWHITE and
AV_PIX_FMT_AYUV64LE, the latter of which amounts to the old limit.

> I still see the problem that this option code does not know which
> pix_fmt the numbers relate to and as such would keep the old limit in
> place despite there being no technical reasons for it.

And I still think that av_image_check_size should be changed to
accept the largest value valid for any pixel format (once every place
needing stricter limits is switched to the pixel format sensitive
check).
Do you disagree with this or what is your point?

Best regards,
Andreas
Hendrik Leppkes Dec. 11, 2016, 8:03 p.m.
On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 11.12.2016 10:04, Hendrik Leppkes wrote:
>> On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun
>> <andreas.cadhalpun@googlemail.com> wrote:
>>> On 10.12.2016 18:40, Hendrik Leppkes wrote:
>>>> and just adds extra burden when these limits are
>>>> changed/improved (say by making them pixfmt aware as discussed in
>>>> another thread, which this function could never know).
>>>
>>> There is no extra burden, because av_image_check_size would have to
>>> be changed in that case anyway to accept the largest value valid
>>> for any pixel format.
>>>
>>
>> av_image_check_size2 was introduced by Michael now which works in the
>> context of a pix_fmt, which for many formats allows significantly
>> larger images then the old function (up to factor 4 larger)
>
> Actually there is a factor of 64 between AV_PIX_FMT_MONOWHITE and
> AV_PIX_FMT_AYUV64LE, the latter of which amounts to the old limit.
>
>> I still see the problem that this option code does not know which
>> pix_fmt the numbers relate to and as such would keep the old limit in
>> place despite there being no technical reasons for it.
>
> And I still think that av_image_check_size should be changed to
> accept the largest value valid for any pixel format (once every place
> needing stricter limits is switched to the pixel format sensitive
> check).
> Do you disagree with this or what is your point?

I could probably live with a simple w*h overflow check (more or less),
which av_image_check_size then probably would end up being if I
understand you right?
Thats much higher then the current limit in most cases and while we
should try to move this to size_t/ptrdiff_t eventually to lift the
limit even higher on 64-bit platforms, its OK for now.

- Hendrik
Andreas Cadhalpun Dec. 11, 2016, 8:45 p.m.
On 11.12.2016 21:03, Hendrik Leppkes wrote:
> On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> On 11.12.2016 10:04, Hendrik Leppkes wrote:
>>> I still see the problem that this option code does not know which
>>> pix_fmt the numbers relate to and as such would keep the old limit in
>>> place despite there being no technical reasons for it.
>>
>> And I still think that av_image_check_size should be changed to
>> accept the largest value valid for any pixel format (once every place
>> needing stricter limits is switched to the pixel format sensitive
>> check).
>> Do you disagree with this or what is your point?
> 
> I could probably live with a simple w*h overflow check (more or less),
> which av_image_check_size then probably would end up being if I
> understand you right?

I don't think so. For example, av_image_check_size2 accepts resolutions
like 100000x80000 for AV_PIX_FMT_MONOWHITE and thus av_image_check_size
should also accept this, even though the number of pixels is larger
than INT_MAX. However, that's not the current state of affairs, so
until the work is done to actually use the pixel format specific limits,
the option code should check for the old limit.

> Thats much higher then the current limit in most cases and while we
> should try to move this to size_t/ptrdiff_t eventually to lift the
> limit even higher on 64-bit platforms, its OK for now.

Note that av_image_check_size is documented to check that
"all bytes of the image can be addressed with a signed int",
so increasing the limit higher requires using a different function.

Best regards,
Andreas
Andreas Cadhalpun Jan. 6, 2017, 7:05 p.m.
On 11.12.2016 21:45, Andreas Cadhalpun wrote:
> On 11.12.2016 21:03, Hendrik Leppkes wrote:
>> On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun
>> <andreas.cadhalpun@googlemail.com> wrote:
>>> On 11.12.2016 10:04, Hendrik Leppkes wrote:
>>>> I still see the problem that this option code does not know which
>>>> pix_fmt the numbers relate to and as such would keep the old limit in
>>>> place despite there being no technical reasons for it.
>>>
>>> And I still think that av_image_check_size should be changed to
>>> accept the largest value valid for any pixel format (once every place
>>> needing stricter limits is switched to the pixel format sensitive
>>> check).
>>> Do you disagree with this or what is your point?
>>
>> I could probably live with a simple w*h overflow check (more or less),
>> which av_image_check_size then probably would end up being if I
>> understand you right?
> 
> I don't think so. For example, av_image_check_size2 accepts resolutions
> like 100000x80000 for AV_PIX_FMT_MONOWHITE and thus av_image_check_size
> should also accept this, even though the number of pixels is larger
> than INT_MAX. However, that's not the current state of affairs, so
> until the work is done to actually use the pixel format specific limits,
> the option code should check for the old limit.
> 
>> Thats much higher then the current limit in most cases and while we
>> should try to move this to size_t/ptrdiff_t eventually to lift the
>> limit even higher on 64-bit platforms, its OK for now.
> 
> Note that av_image_check_size is documented to check that
> "all bytes of the image can be addressed with a signed int",
> so increasing the limit higher requires using a different function.

I assume I've convinced you, so I'll apply this patch soon, unless
I hear back from you.

Best regards,
Andreas

Patch hide | download patch | download mbox

diff --git a/libavutil/opt.c b/libavutil/opt.c
index f855ccb..f713d3f 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -32,6 +32,7 @@ 
 #include "common.h"
 #include "dict.h"
 #include "eval.h"
+#include "imgutils.h"
 #include "log.h"
 #include "parseutils.h"
 #include "pixdesc.h"
@@ -325,8 +326,15 @@  static int set_string_image_size(void *obj, const AVOption *o, const char *val,
         return 0;
     }
     ret = av_parse_video_size(dst, dst + 1, val);
-    if (ret < 0)
+    if (ret < 0) {
         av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val);
+        return ret;
+    }
+    ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
+    if (ret < 0) {
+        *dst = 0;
+        *(dst + 1) = 0;
+    }
     return ret;
 }