[libvirt PATCH] virThreadPoolExpand: Prevent expanding worker pool by zero
`virThreadPoolNewFull` may call `virThreadPoolExpand` with `prioWorkers` = 0. 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@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; + VIR_EXPAND_N(*workers, *curWorkers, gain); for (i = 0; i < gain; i++) { -- 2.31.1
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?
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@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.
+ VIR_EXPAND_N(*workers, *curWorkers, gain);
for (i = 0; i < gain; i++) { -- 2.31.1
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@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. Please let me know your preferences. Regards, Tim
for (i = 0; i < gain; i++) { -- 2.31.1
On 7/12/21 12:57 PM, 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!
I'll just add that this happens by default only for admin server, because it is explicitly called with prio_workers = 0: if (!(srvAdm = virNetServerNew("admin", 1, config->admin_min_workers, config->admin_max_workers, 0, config->admin_max_clients, 0, config->admin_keepalive_interval, config->admin_keepalive_count, remoteAdmClientNew, NULL, remoteAdmClientFree, dmn))) { (see src/remote/remote_daemon.c:1091) But since we allow users to configure number of workers (for other servers too) it may happen with any server, not just admin. I wonder how do you feel about guarding virThreadPoolExpand() with check for #threads > 0, e.g. like this: diff --git i/src/util/virthreadpool.c w/src/util/virthreadpool.c index 9ddd86a679..18660576ef 100644 --- i/src/util/virthreadpool.c +++ w/src/util/virthreadpool.c @@ -250,7 +250,8 @@ virThreadPoolNewFull(size_t minWorkers, if (virThreadPoolExpand(pool, minWorkers, false) < 0) goto error; - if (virThreadPoolExpand(pool, prioWorkers, true) < 0) + if (prioWorkers > 0 && + virThreadPoolExpand(pool, prioWorkers, true) < 0) goto error; return pool; Michal
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@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.
participants (3)
-
Michal Prívozník -
Peter Krempa -
Tim Wiederhake