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

[Philippe Mathieu-Daudé]

On 23/7/24 17:51, Peter Maydell wrote:
> 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.

Ah indeed. Probably very old code hard to change now
(QAPI does support unsigned types).