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

[Michael Niedermayer]

On Tue, Jul 09, 2024 at 04:46:59PM +0200, Kacper Michajlow wrote:
> On Tue, 9 Jul 2024 at 15:17, Anton Khirnov <anton at 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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240709/9e3dde1f/attachment.sig>