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@xxxxxxxxxx> > > > --- > > > 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.
