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

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.

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(a)redhat.com&gt; > > --- > >  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.