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

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
<andreas.cadhalpun at googlemail.com> wrote:
> On 10.12.2016 16:26, wm4 wrote:
>> On Sat, 10 Dec 2016 16:11:15 +0100
>> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>>
>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at 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