Message ID | 20240709113711.1836747-1-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2] avfilter/vf_scale: Cleanup some checks | expand |
Context | Check | Description |
---|---|---|
andriy/configure_x86 | warning | Failed to apply patch |
yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
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?
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;
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 [...]
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 --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 */
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(-)