Assume there's a dnsmasq running (because there's an active virtual network that spawned it). Now, shut down the daemon, remove the dnsmasq binary and start the daemon again. At this point, networkUpdateState() is called, but dnsmasq_caps is NULL (because networkStateInitialize() called earlier failed to set them, rightfully though). Now, the networkUpdateState() tries to read the dnsmasq's PID file using virPidFileReadIfAlive() which takes a path to the corresponding binary as one of its arguments. To provide that path, dnsmasqCapsGetBinaryPath() is called, but since dnsmasq_caps is NULL, it dereferences it and thus causes a crash. It's true that virPidFileReadIfAlive() can deal with a removed binary (well virPidFileReadPathIfAlive() which it calls can), but iff the binary path is provided in its absolute form. Otherwise, virFileResolveAllLinks() fails to canonicalize the path (expected, the path doesn't exist anyway). Therefore, reading dnsmasq's PID file didn't work before v8.1.0-rc1~401 which introduced this crash. It was always set to -1. But passing NULL as binary path instead, makes virPidFileReadIfAlive() return early, right after the PID file is read and it's confirmed the PID exists. Yes, this may yield wrong results, as the PID might be of a completely different binary. But this problem is preexistent and until we start locking PID files, there's nothing we can do about it. IOW, it would require rework of dnsmasq PID file handling. Fixes: 4b68c982e283471575bacbf87302495864da46fe Resolves: https://gitlab.com/libvirt/libvirt/-/issues/456 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 97e099880b..7f2298a15e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -493,15 +493,19 @@ networkUpdateState(virNetworkObj *obj, /* Try and read dnsmasq pids of active networks */ if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { + const char *binpath = NULL; pid_t dnsmasqPid; if (networkSetMacMap(cfg, obj) < 0) return -1; + if (dnsmasq_caps) + binpath = dnsmasqCapsGetBinaryPath(dnsmasq_caps); + ignore_value(virPidFileReadIfAlive(cfg->pidDir, def->name, &dnsmasqPid, - dnsmasqCapsGetBinaryPath(dnsmasq_caps))); + binpath)); virNetworkObjSetDnsmasqPid(obj, dnsmasqPid); } -- 2.39.2

This is a relict of v3.7.0-rc1~132 when getter/setter APIs for dnsmasq's PID were introduced. Previously, obj->dnsmasqPid was accessed directly. But the aforementioned commit introduced two calls to virNetworkObjGetDnsmasqPid() even though the result of the first call is stored in a variable. Remove the second call as it's unnecessary. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7f2298a15e..9eb543a0a3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1599,7 +1599,6 @@ networkRefreshDhcpDaemon(virNetworkDriverState *driver, if (dnsmasqSave(dctx) < 0) return -1; - dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); return kill(dnsmasqPid, SIGHUP); } -- 2.39.2