[FFmpeg-devel] avfilter/scale: fix CID 1457833

---
 libavfilter/vf_scale.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Quoting Gyan Doshi (2020-01-16 13:59:47)
> ---
>  libavfilter/vf_scale.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index d46c767e70..70978345e8 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -498,10 +498,8 @@ static int config_props(AVFilterLink *outlink)
>                                 scale->force_original_aspect_ratio,
>                                 scale->force_divisible_by);
>  
> -    if (scale->w > INT_MAX ||
> -        scale->h > INT_MAX ||
> -        (scale->h * inlink->w) > INT_MAX ||
> -        (scale->w * inlink->h) > INT_MAX)
> +    if (((int64_t)scale->h * inlink->w) > INT_MAX ||
> +        ((int64_t)scale->w * inlink->h) > INT_MAX)

This only works when int is 32bit, which is not guaranteed to be true.
The correct way to test it is something like
if (scale->h > INT_MAX / inlink->w)

Also, the commit message has all the usefulness of "fix bug". It should
say what is actually being fixed.

On 16-01-2020 09:07 pm, Anton Khirnov wrote:
> Quoting Gyan Doshi (2020-01-16 13:59:47)
>> ---
>>   libavfilter/vf_scale.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index d46c767e70..70978345e8 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -498,10 +498,8 @@ static int config_props(AVFilterLink *outlink)
>>                                  scale->force_original_aspect_ratio,
>>                                  scale->force_divisible_by);
>>   
>> -    if (scale->w > INT_MAX ||
>> -        scale->h > INT_MAX ||
>> -        (scale->h * inlink->w) > INT_MAX ||
>> -        (scale->w * inlink->h) > INT_MAX)
>> +    if (((int64_t)scale->h * inlink->w) > INT_MAX ||
>> +        ((int64_t)scale->w * inlink->h) > INT_MAX)
> This only works when int is 32bit, which is not guaranteed to be true.
> The correct way to test it is something like
> if (scale->h > INT_MAX / inlink->w)

Makes sense. I took my cue from similar checks in other scale filters. 
Should convert those too.

Although I do wonder why this check exists at this time. All it results 
in is a log msg; any invalid values aren't adjusted. Should this be 
replaced with av_image_check_size2 or is there another reason for this 
check?

Gyan

Quoting Gyan (2020-01-17 10:40:22)
> 
> 
> On 16-01-2020 09:07 pm, Anton Khirnov wrote:
> > Quoting Gyan Doshi (2020-01-16 13:59:47)
> >> ---
> >>   libavfilter/vf_scale.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> >> index d46c767e70..70978345e8 100644
> >> --- a/libavfilter/vf_scale.c
> >> +++ b/libavfilter/vf_scale.c
> >> @@ -498,10 +498,8 @@ static int config_props(AVFilterLink *outlink)
> >>                                  scale->force_original_aspect_ratio,
> >>                                  scale->force_divisible_by);
> >>   
> >> -    if (scale->w > INT_MAX ||
> >> -        scale->h > INT_MAX ||
> >> -        (scale->h * inlink->w) > INT_MAX ||
> >> -        (scale->w * inlink->h) > INT_MAX)
> >> +    if (((int64_t)scale->h * inlink->w) > INT_MAX ||
> >> +        ((int64_t)scale->w * inlink->h) > INT_MAX)
> > This only works when int is 32bit, which is not guaranteed to be true.
> > The correct way to test it is something like
> > if (scale->h > INT_MAX / inlink->w)
> 
> Makes sense. I took my cue from similar checks in other scale filters. 
> Should convert those too.
> 
> Although I do wonder why this check exists at this time. All it results 
> in is a log msg; any invalid values aren't adjusted. Should this be 
> replaced with av_image_check_size2 or is there another reason for this 
> check?

Yeah, the check seems useless and it's been there since the beginning. I
assume it was intended to avoid overflow in the aspect ratio
calculation.