diff mbox series

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

Message ID 20200116125947.767-1-ffmpeg@gyani.pro
State New
Headers show
Series [FFmpeg-devel] avfilter/scale: fix CID 1457833
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Gyan Doshi Jan. 16, 2020, 12:59 p.m. UTC
---
 libavfilter/vf_scale.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Anton Khirnov Jan. 16, 2020, 3:37 p.m. UTC | #1
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.
Gyan Doshi Jan. 17, 2020, 9:40 a.m. UTC | #2
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
Anton Khirnov Jan. 28, 2020, 12:54 p.m. UTC | #3
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.
diff mbox series

Patch

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)
         av_log(ctx, AV_LOG_ERROR, "Rescaled value for width or height is too big.\n");
 
     outlink->w = scale->w;