Re: [PATCH] util/async.c: Forbid negative min/max in aio_context_set_thread_pool_params()

[Peter Maydell]

On Tue, 23 Jul 2024 at 16:44, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 23/7/24 17:09, Peter Maydell wrote:
> > aio_context_set_thread_pool_params() takes two int64_t arguments to
> > set the minimum and maximum number of threads in the pool.  We do
> > some bounds checking on these, but we don't catch the case where the
> > inputs are negative.  This means that later in the function when we
> > assign these inputs to the AioContext::thread_pool_min and
> > ::thread_pool_max fields, which are of type int, the values might
> > overflow the smaller type.
> >
> > A negative number of threads is meaningless, so make
> > aio_context_set_thread_pool_params() return an error if either min or
> > max are negative.
> >
> > Resolves: Coverity CID 1547605
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   util/async.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/util/async.c b/util/async.c
> > index 0467890052a..3e3e4fc7126 100644
> > --- a/util/async.c
> > +++ b/util/async.c
> > @@ -746,7 +746,7 @@ void aio_context_set_thread_pool_params(AioContext 
> > *ctx, int64_t min,
> >                                           int64_t max, Error **errp)
> >   {
> >
> > -    if (min > max || !max || min > INT_MAX || max > INT_MAX) {
> > +    if (min > max || max <= 0 || min < 0 || min > INT_MAX || max > 
> > INT_MAX) {
> >           error_setg(errp, "bad thread-pool-min/thread-pool-max values");
> >           return;
> >       }
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> I don't get the point of using signed min/max here...

I think this is because those values may originate in a
QAPI command structure (EventLoopBaseProperties), where
they are defined as "int" rather than a specifically
unsigned type. So we carry them around as int64_t until
they get to here, where we do the validation of whether
they're sensible or not.

thanks
-- PMM