[libvirt PATCH] virThreadPoolExpand: Prevent expanding worker pool by zero

[Peter Krempa]

On Mon, Jul 12, 2021 at 12:57:31 +0200, Tim Wiederhake wrote:
> On Mon, 2021-07-12 at 10:09 +0200, Peter Krempa wrote:
> > On Fri, Jul 09, 2021 at 15:43:06 +0200, Tim Wiederhake wrote:
> > > `virThreadPoolNewFull` may call `virThreadPoolExpand` with
> > > `prioWorkers` = 0.
> > 
> > Could you elaborate in which situations this happens?
> 
> Sure!
> 
> On libvirtd startup, the list of priority worker threads is
> uninitialized (`pool->prioWorkers` is NULL in `virThreadPoolNewFull`),
> and then "expanded" to zero (`prioWorkers`) entries.

Please mention this in the commit message that it's an actually occuring
problem.

> 
> This can be triggered by:
> 
>     $ meson -Dbuildtype=debug -Db_lundef=false -
> Db_sanitize=address,undefined build
>     $ ninja -C build
>     $ ./build/run build/src/libvirtd
>     ../src/util/viralloc.c:79:5: runtime error: null pointer passed as
> argument 1, which is declared to never be null
> 
> Note that this happens directly on startup, even before the "libvirt
> version" and "hostname" output, and no client connection is required.
> 
> > > This causes `virThreadPoolExpand` to call `VIR_EXPAND_N` on a null
> > > pointer
> > > and an increment of zero. The zero increment triggers `virReallocN`
> > > to not
> > > actually allocate any memory and leave the pointer NULL, which,
> > > eventually,
> > > causes `memset(NULL, 0, 0)` to be called in `virExpandN`.
> > > 
> > > `memset` is declared `__attribute__ ((__nonnull__ 1))`, which
> > > triggers the
> > > following warning when libvirt is compiled with address sanitizing
> > > enabled:
> > > 
> > >     src/util/viralloc.c:82:5: runtime error: null pointer passed as
> > >     argument 1, which is declared to never be null
> > > 
> > > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > > ---
> > >  src/util/virthreadpool.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
> > > index 9ddd86a679..c9d2a17ff4 100644
> > > --- a/src/util/virthreadpool.c
> > > +++ b/src/util/virthreadpool.c
> > > @@ -179,6 +179,9 @@ virThreadPoolExpand(virThreadPool *pool, size_t
> > > gain, bool priority)
> > >      size_t i = 0;
> > >      struct virThreadPoolWorkerData *data = NULL;
> > >  
> > > +    if (gain == 0)
> > > +        return 0;
> > 
> > IMO this is fixing a symptom rather than a root cause unless you
> > justify
> > it.
> 
> Sorry, I don't follow. This guards against calling `VIR_EXPAND_N` on an
> uninitialized list with 0 increment...
> 
> > > +    VIR_EXPAND_N(*workers, *curWorkers, gain);
> 
> ... here, in the very next line.
> 
> I would be happy to fix this in virExpandN ...
> 
>     diff --git a/src/util/viralloc.c b/src/util/viralloc.c
>     index cd7ae9e7d1..41c7cf22a6 100644
>     --- a/src/util/viralloc.c
>     +++ b/src/util/viralloc.c
>     @@ -76,7 +76,8 @@ void virExpandN(void *ptrptr,
>              abort();
>  
>          virReallocN(ptrptr, size, *countptr + add);
>     -    memset(*(char **)ptrptr + (size * *countptr), 0, size * add);
>     +    if (*(char **)ptrptr)
>     +        memset(*(char **)ptrptr + (size * *countptr), 0, size *
> add);
>          *countptr += add;
>      }
>  
> ... but that introduces a branch (that is almost always taken) into a
> code path that is executed way more often than the one time the worker
> pool is set up. A check for `add == 0` would be overdoing things, as a
> zero `add` is not problematic if the list is initialized.
> 
> Another way to fix this would be adding a guard in
> `virThreadPoolNewFull`...
> 
>     diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
>     index 9ddd86a679..f04d5a3f0a 100644
>     --- a/src/util/virthreadpool.c
>     +++ b/src/util/virthreadpool.c
>     @@ -247,10 +247,10 @@ virThreadPoolNewFull(size_t minWorkers,
>          pool->maxWorkers = maxWorkers;
>          pool->maxPrioWorkers = prioWorkers;
>      
>     -    if (virThreadPoolExpand(pool, minWorkers, false) < 0)
>     +    if (minWorkers && virThreadPoolExpand(pool, minWorkers, false)
> < 0)
>              goto error;
>      
>     -    if (virThreadPoolExpand(pool, prioWorkers, true) < 0)
>     +    if (prioWorkers && virThreadPoolExpand(pool, prioWorkers,
> true) < 0)
>              goto error;
>      
>          return pool;
> 
> ... but this does not guard against potential later "zero expansions"
> on the still NULL array.

I meant this. A zero expansion doesn't make much sense in the first
place so adding code to special case it and do nothing feels weird.