diff mbox series

[FFmpeg-devel,v2] avfilter/vf_scale: Cleanup some checks

Message ID 20240709113711.1836747-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,v2] avfilter/vf_scale: Cleanup some checks | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to apply patch
yinshiyou/configure_loongarch64 warning Failed to apply patch

Commit Message

Michael Niedermayer July 9, 2024, 11:37 a.m. UTC 
Fixes: CID1513722 Operands don't affect result

Sponsored-by: Sovereign Tech Fund
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavfilter/vf_scale.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Anton Khirnov July 9, 2024, 1:17 p.m. UTC | #1 
Quoting Michael Niedermayer (2024-07-09 13:37:11)
> Fixes: CID1513722 Operands don't affect result
> 
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  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 bf09196e10d..18e9393d6c1 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -645,10 +645,8 @@ static int config_props(AVFilterLink *outlink)
>      if (ret < 0)
>          goto fail;
>  
> -    if (outlink->w > INT_MAX ||
> -        outlink->h > INT_MAX ||
> -        (outlink->h * inlink->w) > INT_MAX ||
> -        (outlink->w * inlink->h) > INT_MAX)
> +    if ((outlink->h * (int64_t)inlink->w) > INT32_MAX ||
> +        (outlink->w * (int64_t)inlink->h) > INT32_MAX)

This does not seems cleaner to me.

Also, this check overall seems fishy. Why is it here at all and not e.g.
in ff_scale_adjust_dimensions()? Why does it not call
av_image_check_size()? Why does it only print a warning and not do
anything else?
Kacper Michajlow July 9, 2024, 2:46 p.m. UTC | #2 
On Tue, 9 Jul 2024 at 15:17, Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Michael Niedermayer (2024-07-09 13:37:11)
> > Fixes: CID1513722 Operands don't affect result
> >
> > Sponsored-by: Sovereign Tech Fund
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  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 bf09196e10d..18e9393d6c1 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -645,10 +645,8 @@ static int config_props(AVFilterLink *outlink)
> >      if (ret < 0)
> >          goto fail;
> >
> > -    if (outlink->w > INT_MAX ||
> > -        outlink->h > INT_MAX ||
> > -        (outlink->h * inlink->w) > INT_MAX ||
> > -        (outlink->w * inlink->h) > INT_MAX)
> > +    if ((outlink->h * (int64_t)inlink->w) > INT32_MAX ||
> > +        (outlink->w * (int64_t)inlink->h) > INT32_MAX)
>
> This does not seems cleaner to me.
>
> Also, this check overall seems fishy. Why is it here at all and not e.g.
> in ff_scale_adjust_dimensions()? Why does it not call
> av_image_check_size()? Why does it only print a warning and not do
> anything else?

I agree with Anton here. The checks in vf_scale are iffy at best.

For starters, this is `outlink->w > INT_MAX` dead check. As the `w` is
int already. And there is already an UB in `scale_eval_dimensions()`
which converts double value to int without any checks.

I think something like this would make sense to reject big numbers,
and ofcourse ff_scale_adjust_dimensions() should be more clever about
overflows too. There is also an overflow in swscale, but that's
another story.

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index a1a322ed9e..9483db7564 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -537,7 +537,7 @@ static int scale_eval_dimensions(AVFilterContext *ctx)
const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
char *expr;
- int eval_w, eval_h;
+ double eval_w, eval_h;
int ret;
double res;
const AVPixFmtDescriptor *main_desc;
@@ -588,23 +588,22 @@ static int scale_eval_dimensions(AVFilterContext *ctx)
}

res = av_expr_eval(scale->w_pexpr, scale->var_values, NULL);
- eval_w = scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] =
(int) res == 0 ? inlink->w : (int) res;
+ eval_w = scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] =
res == 0 ? inlink->w : res;

res = av_expr_eval(scale->h_pexpr, scale->var_values, NULL);
- if (isnan(res)) {
- expr = scale->h_expr;
+ eval_h = scale->var_values[VAR_OUT_H] = scale->var_values[VAR_OH] =
res == 0 ? inlink->h : res;
+
+ if (isnan(eval_w) || eval_w < INT_MIN || eval_w > INT_MAX) {
+ expr = scale->w_expr;
ret = AVERROR(EINVAL);
goto fail;
}
- eval_h = scale->var_values[VAR_OUT_H] = scale->var_values[VAR_OH] =
(int) res == 0 ? inlink->h : (int) res;

- res = av_expr_eval(scale->w_pexpr, scale->var_values, NULL);
- if (isnan(res)) {
- expr = scale->w_expr;
+ if (isnan(eval_h) || eval_h < INT_MIN || eval_h > INT_MAX) {
+ expr = scale->h_expr;
ret = AVERROR(EINVAL);
goto fail;
}
- eval_w = scale->var_values[VAR_OUT_W] = scale->var_values[VAR_OW] =
(int) res == 0 ? inlink->w : (int) res;

scale->w = eval_w;
scale->h = eval_h;
Michael Niedermayer July 9, 2024, 9:43 p.m. UTC | #3 
On Tue, Jul 09, 2024 at 04:46:59PM +0200, Kacper Michajlow wrote:
> On Tue, 9 Jul 2024 at 15:17, Anton Khirnov <anton@khirnov.net> wrote:
> >
> > Quoting Michael Niedermayer (2024-07-09 13:37:11)
> > > Fixes: CID1513722 Operands don't affect result
> > >
> > > Sponsored-by: Sovereign Tech Fund
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  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 bf09196e10d..18e9393d6c1 100644
> > > --- a/libavfilter/vf_scale.c
> > > +++ b/libavfilter/vf_scale.c
> > > @@ -645,10 +645,8 @@ static int config_props(AVFilterLink *outlink)
> > >      if (ret < 0)
> > >          goto fail;
> > >
> > > -    if (outlink->w > INT_MAX ||
> > > -        outlink->h > INT_MAX ||
> > > -        (outlink->h * inlink->w) > INT_MAX ||
> > > -        (outlink->w * inlink->h) > INT_MAX)
> > > +    if ((outlink->h * (int64_t)inlink->w) > INT32_MAX ||
> > > +        (outlink->w * (int64_t)inlink->h) > INT32_MAX)
> >
> > This does not seems cleaner to me.
> >
> > Also, this check overall seems fishy. Why is it here at all and not e.g.
> > in ff_scale_adjust_dimensions()? Why does it not call
> > av_image_check_size()? Why does it only print a warning and not do
> > anything else?

I intend to post a better version, but iam a little overworked ATM
so not sure if someone else will do it first.


> 
> I agree with Anton here. The checks in vf_scale are iffy at best.

> For starters, this is `outlink->w > INT_MAX` dead check. As the `w` is
> int already.

that was removed by my patch for that reason. The issue btw originated
by code factorization that replaced int64 by int IIRC


> And there is already an UB in `scale_eval_dimensions()`
> which converts double value to int without any checks.

i try to work on one issue at a time ...


> 
> I think something like this would make sense to reject big numbers,
> and ofcourse ff_scale_adjust_dimensions() should be more clever about
> overflows too. There is also an overflow in swscale, but that's
> another story.
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index a1a322ed9e..9483db7564 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -537,7 +537,7 @@ static int scale_eval_dimensions(AVFilterContext *ctx)
> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
> char *expr;
> - int eval_w, eval_h;
> + double eval_w, eval_h;
> int ret;
> double res;
> const AVPixFmtDescriptor *main_desc;

not a valid patch, that wont apply, its also too messed up formating wise
to review

also we generally want to minimize double/float so any switch from int to double
generally feels "wrong"

thx

[...]
Kacper Michajlow July 10, 2024, 3:27 p.m. UTC | #4 
On Tue, 9 Jul 2024 at 23:43, Michael Niedermayer <michael@niedermayer.cc> wrote:
>
> On Tue, Jul 09, 2024 at 04:46:59PM +0200, Kacper Michajlow wrote:
> > On Tue, 9 Jul 2024 at 15:17, Anton Khirnov <anton@khirnov.net> wrote:
> > >
> > > Quoting Michael Niedermayer (2024-07-09 13:37:11)
> > > > Fixes: CID1513722 Operands don't affect result
> > > >
> > > > Sponsored-by: Sovereign Tech Fund
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  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 bf09196e10d..18e9393d6c1 100644
> > > > --- a/libavfilter/vf_scale.c
> > > > +++ b/libavfilter/vf_scale.c
> > > > @@ -645,10 +645,8 @@ static int config_props(AVFilterLink *outlink)
> > > >      if (ret < 0)
> > > >          goto fail;
> > > >
> > > > -    if (outlink->w > INT_MAX ||
> > > > -        outlink->h > INT_MAX ||
> > > > -        (outlink->h * inlink->w) > INT_MAX ||
> > > > -        (outlink->w * inlink->h) > INT_MAX)
> > > > +    if ((outlink->h * (int64_t)inlink->w) > INT32_MAX ||
> > > > +        (outlink->w * (int64_t)inlink->h) > INT32_MAX)
> > >
> > > This does not seems cleaner to me.
> > >
> > > Also, this check overall seems fishy. Why is it here at all and not e.g.
> > > in ff_scale_adjust_dimensions()? Why does it not call
> > > av_image_check_size()? Why does it only print a warning and not do
> > > anything else?
>
> I intend to post a better version, but iam a little overworked ATM
> so not sure if someone else will do it first.
>
>
> >
> > I agree with Anton here. The checks in vf_scale are iffy at best.
>
> > For starters, this is `outlink->w > INT_MAX` dead check. As the `w` is
> > int already.
>
> that was removed by my patch for that reason. The issue btw originated
> by code factorization that replaced int64 by int IIRC
>
>
> > And there is already an UB in `scale_eval_dimensions()`
> > which converts double value to int without any checks.
>
> i try to work on one issue at a time ...
>
>
> >
> > I think something like this would make sense to reject big numbers,
> > and ofcourse ff_scale_adjust_dimensions() should be more clever about
> > overflows too. There is also an overflow in swscale, but that's
> > another story.
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index a1a322ed9e..9483db7564 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -537,7 +537,7 @@ static int scale_eval_dimensions(AVFilterContext *ctx)
> > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> > const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
> > char *expr;
> > - int eval_w, eval_h;
> > + double eval_w, eval_h;
> > int ret;
> > double res;
> > const AVPixFmtDescriptor *main_desc;
>
> not a valid patch, that wont apply, its also too messed up formating wise
> to review

Ah, sorry, I wanted to just point at the issue, with quick diff, but
my client destroyed it. I've sent a properly formatted patch now, as I
feel it is a good change to do.

- Kacper
diff mbox series

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index bf09196e10d..18e9393d6c1 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -645,10 +645,8 @@  static int config_props(AVFilterLink *outlink)
     if (ret < 0)
         goto fail;
 
-    if (outlink->w > INT_MAX ||
-        outlink->h > INT_MAX ||
-        (outlink->h * inlink->w) > INT_MAX ||
-        (outlink->w * inlink->h) > INT_MAX)
+    if ((outlink->h * (int64_t)inlink->w) > INT32_MAX ||
+        (outlink->w * (int64_t)inlink->h) > INT32_MAX)
         av_log(ctx, AV_LOG_ERROR, "Rescaled value for width or height is too big.\n");
 
     /* TODO: make algorithm configurable */