[PATCH v2 0/9] ix double free in URI alias lookup and fix misleading error with libvirtd
When libvirtd is built without a driver we report a terrible error message which doesn't really point to what's happening. v2: - propagate that we want to avoid the remote driver internally rather than via a public API flag Peter Krempa (9): virConnectOpenInternal: Avoid double free() when alias is an invalid URI virConnectOpenInternal: Switch to automatic memory cleanup virConnectOpenInternal: Remove 'failed' label remote: remoteOpenConn: Use virConnectOpenAuth instead of virConnectOpen(ReadOnly) remoteConnectOpen: Refactor cleanup remote: doRemoteOpen: Automatically clean up 'priv' lxc: Remove unneeded forward declaration of 'lxcStateInitialize' virStateInitialize: Propagate whether running in monolithic daemon mode to stateful driver init remote: Don't attempt remote connection from libvirtd src/bhyve/bhyve_driver.c | 1 + src/ch/ch_driver.c | 1 + src/driver-state.h | 1 + src/interface/interface_backend_netcf.c | 1 + src/interface/interface_backend_udev.c | 1 + src/libvirt.c | 67 +++++++++++-------------- src/libvirt_internal.h | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 5 +- src/network/bridge_driver.c | 1 + src/node_device/node_device_udev.c | 1 + src/nwfilter/nwfilter_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon.c | 6 +++ src/remote/remote_daemon_dispatch.c | 7 ++- src/remote/remote_driver.c | 59 +++++++++++----------- src/secret/secret_driver.c | 1 + src/storage/storage_driver.c | 1 + src/vz/vz_driver.c | 1 + 19 files changed, 83 insertions(+), 75 deletions(-) -- 2.37.1
Configuring an URI alias such as uri_aliases = [ "blah=qemu://invaliduri@@@", ] Results in a double free when the alias is used: $ virsh -c blah free(): double free detected in tcache 2 Aborted (core dumped) This happens as the 'alias' variable is first assigned to 'uristr' which is cleared in the 'failed' label and then is explicitly freed again. Fix this by switching to 'g_autofree' for alias and stealing it into 'uristr'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index b78b49a632..7e7c0fc8e3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -933,21 +933,19 @@ virConnectOpenInternal(const char *name, } if (uristr) { - char *alias = NULL; + g_autofree char *alias = NULL; if (!(flags & VIR_CONNECT_NO_ALIASES) && virURIResolveAlias(conf, uristr, &alias) < 0) goto failed; if (alias) { - VIR_FREE(uristr); - uristr = alias; + g_free(uristr); + uristr = g_steal_pointer(&alias); } - if (!(ret->uri = virURIParse(uristr))) { - VIR_FREE(alias); + if (!(ret->uri = virURIParse(uristr))) goto failed; - } /* Avoid need for drivers to worry about NULLs, as * no one needs to distinguish "" vs NULL */ -- 2.37.1
On a Friday in 2022, Peter Krempa wrote:
Configuring an URI alias such as
uri_aliases = [ "blah=qemu://invaliduri@@@", ]
Results in a double free when the alias is used:
$ virsh -c blah free(): double free detected in tcache 2 Aborted (core dumped)
This happens as the 'alias' variable is first assigned to 'uristr' which is cleared in the 'failed' label and then is explicitly freed again.
Fix this by switching to 'g_autofree' for alias and stealing it into 'uristr'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b78b49a632..7e7c0fc8e3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -933,21 +933,19 @@ virConnectOpenInternal(const char *name, }
if (uristr) { - char *alias = NULL; + g_autofree char *alias = NULL;
Is this necessary? In the case 'alias' will be assigned a value, we will steal the value right away.
if (!(flags & VIR_CONNECT_NO_ALIASES) && virURIResolveAlias(conf, uristr, &alias) < 0) goto failed;
if (alias) { - VIR_FREE(uristr); - uristr = alias; + g_free(uristr); + uristr = g_steal_pointer(&alias); }
- if (!(ret->uri = virURIParse(uristr))) { - VIR_FREE(alias); + if (!(ret->uri = virURIParse(uristr))) goto failed; - }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use automatic cleanup for 'ret' and 'uristr'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 7e7c0fc8e3..fca38dba40 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -882,9 +882,9 @@ virConnectOpenInternal(const char *name, { size_t i; int res; - virConnectPtr ret; + g_autoptr(virConnect) ret = NULL; g_autoptr(virConf) conf = NULL; - char *uristr = NULL; + g_autofree char *uristr = NULL; bool embed = false; ret = virGetConnect(); @@ -1151,14 +1151,9 @@ virConnectOpenInternal(const char *name, goto failed; } - VIR_FREE(uristr); - - return ret; + return g_steal_pointer(&ret); failed: - VIR_FREE(uristr); - virObjectUnref(ret); - return NULL; } -- 2.37.1
Jumping to the label would just return NULL. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index fca38dba40..d5ae68d16f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -892,7 +892,7 @@ virConnectOpenInternal(const char *name, return NULL; if (virConfLoadConfig(&conf, "libvirt.conf") < 0) - goto failed; + return NULL; if (name && name[0] == '\0') name = NULL; @@ -916,14 +916,14 @@ virConnectOpenInternal(const char *name, uristr = g_strdup(name); } else { if (virConnectGetDefaultURI(conf, &uristr) < 0) - goto failed; + return NULL; if (uristr == NULL) { VIR_DEBUG("Trying to probe for default URI"); for (i = 0; i < virConnectDriverTabCount && uristr == NULL; i++) { if (virConnectDriverTab[i]->hypervisorDriver->connectURIProbe) { if (virConnectDriverTab[i]->hypervisorDriver->connectURIProbe(&uristr) < 0) - goto failed; + return NULL; VIR_DEBUG("%s driver URI probe returned '%s'", virConnectDriverTab[i]->hypervisorDriver->name, NULLSTR(uristr)); @@ -937,7 +937,7 @@ virConnectOpenInternal(const char *name, if (!(flags & VIR_CONNECT_NO_ALIASES) && virURIResolveAlias(conf, uristr, &alias) < 0) - goto failed; + return NULL; if (alias) { g_free(uristr); @@ -945,7 +945,7 @@ virConnectOpenInternal(const char *name, } if (!(ret->uri = virURIParse(uristr))) - goto failed; + return NULL; /* Avoid need for drivers to worry about NULLs, as * no one needs to distinguish "" vs NULL */ @@ -967,12 +967,12 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_NO_CONNECT, _("URI '%s' does not include a driver name"), name); - goto failed; + return NULL; } if (virConnectCheckURIMissingSlash(uristr, ret->uri) < 0) { - goto failed; + return NULL; } if (STREQ(ret->uri->path, "/embed")) { @@ -985,26 +985,26 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_NO_CONNECT, _("URI scheme '%s' for embedded driver is not valid"), ret->uri->scheme); - goto failed; + return NULL; } root = virURIGetParam(ret->uri, "root"); if (!root) - goto failed; + return NULL; if (!g_path_is_absolute(root)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("root path must be absolute")); - goto failed; + return NULL; } if (virEventRequireImpl() < 0) - goto failed; + return NULL; regMethod = g_strdup_printf("%sRegister", ret->uri->scheme); if (virDriverLoadModule(ret->uri->scheme, regMethod, false) < 0) - goto failed; + return NULL; if (virAccessManagerGetDefault() == NULL) { virAccessManager *acl; @@ -1012,12 +1012,12 @@ virConnectOpenInternal(const char *name, virResetLastError(); if (!(acl = virAccessManagerNew("none"))) - goto failed; + return NULL; virAccessManagerSetDefault(acl); } if (virStateInitialize(geteuid() == 0, true, root, NULL, NULL) < 0) - goto failed; + return NULL; embed = true; } @@ -1055,7 +1055,7 @@ virConnectOpenInternal(const char *name, __FILE__, __FUNCTION__, __LINE__, _("libvirt was built without the '%s' driver"), ret->uri->scheme); - goto failed; + return NULL; } VIR_DEBUG("trying driver %zu (%s) ...", @@ -1105,13 +1105,13 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Driver %s cannot be used in embedded mode"), virConnectDriverTab[i]->hypervisorDriver->name); - goto failed; + return NULL; } /* before starting the new connection, check if the driver only works * with a server, and so return an error if the server is missing */ if (virConnectDriverTab[i]->remoteOnly && ret->uri && !ret->uri->server) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("URI is missing the server part")); - goto failed; + return NULL; } ret->driver = virConnectDriverTab[i]->hypervisorDriver; @@ -1141,20 +1141,17 @@ virConnectOpenInternal(const char *name, ret->storageDriver = NULL; if (res == VIR_DRV_OPEN_ERROR) - goto failed; + return NULL; } } if (!ret->driver) { /* If we reach here, then all drivers declined the connection. */ virReportError(VIR_ERR_NO_CONNECT, "%s", NULLSTR(name)); - goto failed; + return NULL; } return g_steal_pointer(&ret); - - failed: - return NULL; } -- 2.37.1
virConnectOpenAuth provides an unified interface with using 'flags' to select the proper mode. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_daemon_dispatch.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index dc5790f077..061e0f7811 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1790,6 +1790,7 @@ remoteOpenConn(const char *uri, { g_autoptr(virTypedParamList) identparams = NULL; g_autoptr(virConnect) newconn = NULL; + unsigned int connectFlags = 0; VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p", NULLSTR(uri), readonly, preserveIdentity, conn); @@ -1814,11 +1815,9 @@ remoteOpenConn(const char *uri, VIR_DEBUG("Opening driver %s", uri); if (readonly) - newconn = virConnectOpenReadOnly(uri); - else - newconn = virConnectOpen(uri); + connectFlags |= VIR_CONNECT_RO; - if (!newconn) + if (!(newconn = virConnectOpenAuth(uri, NULL, connectFlags))) return -1; VIR_DEBUG("Opened driver %p", newconn); -- 2.37.1
Use automatic memory freeing for 'driver' and return error right away to avoid the 'cleanup' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_driver.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 0ca365c4cc..b670284211 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1228,23 +1228,20 @@ remoteConnectOpen(virConnectPtr conn, struct private_data *priv; int ret = VIR_DRV_OPEN_ERROR; unsigned int rflags = 0; - char *driver = NULL; + g_autofree char *driver = NULL; remoteDriverTransport transport; if (conn->uri) { if (remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) - goto cleanup; + return VIR_DRV_OPEN_ERROR; } else { /* No URI, then must be probing so use UNIX socket */ transport = REMOTE_DRIVER_TRANSPORT_UNIX; } - if (inside_daemon) { - if (!conn->uri) { - ret = VIR_DRV_OPEN_DECLINED; - goto cleanup; - } + if (!conn->uri) + return VIR_DRV_OPEN_DECLINED; /* If there's a driver registered we must defer to that. * If there isn't a driver, we must connect in "direct" @@ -1254,15 +1251,12 @@ remoteConnectOpen(virConnectPtr conn, * host */ if (!conn->uri->server && virHasDriverForURIScheme(driver) && - !virURICheckUnixSocket(conn->uri)) { - ret = VIR_DRV_OPEN_DECLINED; - goto cleanup; - } + !virURICheckUnixSocket(conn->uri)) + return VIR_DRV_OPEN_DECLINED; } if (!(priv = remoteAllocPrivateData())) - goto cleanup; - + return VIR_DRV_OPEN_ERROR; remoteGetURIDaemonInfo(conn->uri, transport, &rflags); if (flags & VIR_CONNECT_RO) @@ -1278,8 +1272,6 @@ remoteConnectOpen(virConnectPtr conn, remoteDriverUnlock(priv); } - cleanup: - VIR_FREE(driver); return ret; } -- 2.37.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_driver.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b670284211..25c80a09c7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1225,7 +1225,7 @@ remoteConnectOpen(virConnectPtr conn, virConf *conf, unsigned int flags) { - struct private_data *priv; + g_autofree struct private_data *priv = NULL; int ret = VIR_DRV_OPEN_ERROR; unsigned int rflags = 0; g_autofree char *driver = NULL; @@ -1263,14 +1263,12 @@ remoteConnectOpen(virConnectPtr conn, rflags |= REMOTE_DRIVER_OPEN_RO; ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags); - if (ret != VIR_DRV_OPEN_SUCCESS) { + remoteDriverUnlock(priv); + + if (ret != VIR_DRV_OPEN_SUCCESS) conn->privateData = NULL; - remoteDriverUnlock(priv); - VIR_FREE(priv); - } else { - conn->privateData = priv; - remoteDriverUnlock(priv); - } + else + conn->privateData = g_steal_pointer(&priv); return ret; } -- 2.37.1
The function is used only after the definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_driver.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e7c6c4fbc4..f2a358805a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -79,10 +79,6 @@ VIR_LOG_INIT("lxc.lxc_driver"); #define LXC_NB_DOMAIN_BLOCK_STAT_PARAM 4 -static int lxcStateInitialize(bool privileged, - const char *root, - virStateInhibitCallback callback, - void *opaque); static int lxcStateCleanup(void); virLXCDriver *lxc_driver = NULL; -- 2.37.1
Upcoming patch which is fixing the opening of drivers in monolithic mode needs to know whether we are inside 'libvirtd' but the code where the decision needs to happen is not re-compiled per daemon. Thus we need to pass this information to the stateful driver init function so that it can be remebered. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/bhyve/bhyve_driver.c | 1 + src/ch/ch_driver.c | 1 + src/driver-state.h | 1 + src/interface/interface_backend_netcf.c | 1 + src/interface/interface_backend_udev.c | 1 + src/libvirt.c | 5 ++++- src/libvirt_internal.h | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/network/bridge_driver.c | 1 + src/node_device/node_device_udev.c | 1 + src/nwfilter/nwfilter_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon.c | 6 ++++++ src/remote/remote_driver.c | 1 + src/secret/secret_driver.c | 1 + src/storage/storage_driver.c | 1 + src/vz/vz_driver.c | 1 + 18 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 09ba52483a..e0bf2a19a6 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1178,6 +1178,7 @@ bhyveStateCleanup(void) static int bhyveStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index c6e92efb2c..db2a66d131 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -861,6 +861,7 @@ static int chStateCleanup(void) static int chStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/driver-state.h b/src/driver-state.h index f3a0638e90..7f8b61fa1c 100644 --- a/src/driver-state.h +++ b/src/driver-state.h @@ -33,6 +33,7 @@ typedef enum { typedef virDrvStateInitResult (*virDrvStateInitialize)(bool privileged, const char *root, + bool monolithic, virStateInhibitCallback callback, void *opaque); diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index 90514692e2..5964720e0f 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -89,6 +89,7 @@ virNetcfDriverStateDispose(void *obj) static int netcfStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 32318d8d50..979f187d87 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -1110,6 +1110,7 @@ udevStateCleanup(void); static int udevStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/libvirt.c b/src/libvirt.c index d5ae68d16f..1b4c90e110 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -604,6 +604,7 @@ virRegisterStateDriver(virStateDriver *driver) * @privileged: set to true if running with root privilege, false otherwise * @mandatory: set to true if all drivers must report success, not skipped * @root: directory to use for embedded mode + * @monolithic: set to true if running in monolithic mode (daemon is libvirtd) * @callback: callback to invoke to inhibit shutdown of the daemon * @opaque: data to pass to @callback * @@ -633,6 +634,7 @@ int virStateInitialize(bool privileged, bool mandatory, const char *root, + bool monolithic, virStateInhibitCallback callback, void *opaque) { @@ -650,6 +652,7 @@ virStateInitialize(bool privileged, virStateDriverTab[i]->initialized = true; ret = virStateDriverTab[i]->stateInitialize(privileged, root, + monolithic, callback, opaque); VIR_DEBUG("State init result %d (mandatory=%d)", ret, mandatory); @@ -1016,7 +1019,7 @@ virConnectOpenInternal(const char *name, virAccessManagerSetDefault(acl); } - if (virStateInitialize(geteuid() == 0, true, root, NULL, NULL) < 0) + if (virStateInitialize(geteuid() == 0, true, root, false, NULL, NULL) < 0) return NULL; embed = true; diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index f4e592922d..1ae3e2b2e0 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -32,6 +32,7 @@ typedef void (*virStateInhibitCallback)(bool inhibit, int virStateInitialize(bool privileged, bool mandatory, const char *root, + bool monolithic, virStateInhibitCallback inhibit, void *opaque); int virStateShutdownPrepare(void); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 79af2f4441..1b8b40e9e0 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -648,6 +648,7 @@ libxlAddDom0(libxlDriverPrivate *driver) static int libxlStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback, void *opaque) { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f2a358805a..d66c26221c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1430,6 +1430,7 @@ lxcSecurityInit(virLXCDriverConfig *cfg) static int lxcStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7c7812e276..7c6430b4e3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -575,6 +575,7 @@ firewalld_dbus_signal_callback(GDBusConnection *connection G_GNUC_UNUSED, static int networkStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a06eaade5d..07c10f0d88 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2226,6 +2226,7 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, static int nodeStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d1bdd806fc..9cb306909c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -211,6 +211,7 @@ nwfilterStateCleanup(void) static int nwfilterStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6931f0670d..94b70872d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -549,6 +549,7 @@ qemuDomainFindMaxID(virDomainObj *vm, static int qemuStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback, void *opaque) { diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 36d95de83d..f369d09d35 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -591,6 +591,11 @@ static void daemonRunStateInit(void *opaque) #else /* ! MODULE_NAME */ bool mandatory = false; #endif /* ! MODULE_NAME */ +#ifdef LIBVIRTD + bool monolithic = true; +#else /* ! LIBVIRTD */ + bool monolithic = false; +#endif /* ! LIBVIRTD */ virIdentitySetCurrent(sysident); @@ -605,6 +610,7 @@ static void daemonRunStateInit(void *opaque) if (virStateInitialize(virNetDaemonIsPrivileged(dmn), mandatory, NULL, + monolithic, daemonInhibitCallback, dmn) < 0) { VIR_ERROR(_("Driver state initialization failed")); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 25c80a09c7..33cc6b1fce 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -168,6 +168,7 @@ static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho static int remoteStateInitialize(bool privileged G_GNUC_UNUSED, const char *root G_GNUC_UNUSED, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 080ed962a9..6328589fa4 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -462,6 +462,7 @@ secretStateCleanup(void) static int secretStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8dc2cfa7c9..fccf0fcf52 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -242,6 +242,7 @@ storageDriverAutostart(void) static int storageStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 571d895167..4f5e340d53 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -4077,6 +4077,7 @@ vzStateCleanup(void) static int vzStateInitialize(bool privileged, const char *root, + bool monolithic G_GNUC_UNUSED, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { -- 2.37.1
When a hypervisor driver is not compiled in and a user enables the monolithic libvirtd, they get the following misleading error: $ virsh -c qemu:///system error: failed to connect to the hypervisor error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': No such file or directory The issue is that the daemon side of the remote driver can't find the appropriate driver, but the remote driver always accepts everything and thus attempts to delegate further, which in case of libvirtd makes no sense. Refuse opening a connection for local URIS even when the requested driver is not registered in case when we are inside 'libvirtd' as libvirtd doesn't have anything to delegate to. $ virsh -c qemu:///system error: failed to connect to the hypervisor error: no connection driver available for qemu:///system Discovered when investigating https://gitlab.com/libvirt/libvirt/-/issues/370 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_driver.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 33cc6b1fce..a4efe101a3 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -73,6 +73,7 @@ VIR_LOG_INIT("remote.remote_driver"); #endif static bool inside_daemon; +static bool monolithic_daemon; struct private_data { virMutex lock; @@ -168,7 +169,7 @@ static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho static int remoteStateInitialize(bool privileged G_GNUC_UNUSED, const char *root G_GNUC_UNUSED, - bool monolithic G_GNUC_UNUSED, + bool monolithic, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { @@ -176,6 +177,7 @@ remoteStateInitialize(bool privileged G_GNUC_UNUSED, * re-entering ourselves */ inside_daemon = true; + monolithic_daemon = monolithic; return VIR_DRV_STATE_INIT_COMPLETE; } @@ -1244,16 +1246,22 @@ remoteConnectOpen(virConnectPtr conn, if (!conn->uri) return VIR_DRV_OPEN_DECLINED; - /* If there's a driver registered we must defer to that. - * If there isn't a driver, we must connect in "direct" - * mode - see doRemoteOpen. - * One exception is if we are trying to connect to an - * unknown socket path as that might be proxied to remote - * host */ - if (!conn->uri->server && - virHasDriverForURIScheme(driver) && - !virURICheckUnixSocket(conn->uri)) - return VIR_DRV_OPEN_DECLINED; + /* Handle deferring to local drivers if we are dealing with a default + * local URI. (Unknown local socket paths may be proxied to a remote + * host so they are treated as remote too). + * + * Deferring to a local driver is needed if: + * - the driver is registered in the current daemon + * - if we are running monolithic libvirtd, in which case we consider + * even un-registered drivers as local + */ + if (!conn->uri->server && !virURICheckUnixSocket(conn->uri)) { + if (virHasDriverForURIScheme(driver)) + return VIR_DRV_OPEN_DECLINED; + + if (monolithic_daemon) + return VIR_DRV_OPEN_DECLINED; + } } if (!(priv = remoteAllocPrivateData())) -- 2.37.1
On a Friday in 2022, Peter Krempa wrote:
When libvirtd is built without a driver we report a terrible error message which doesn't really point to what's happening.
v2: - propagate that we want to avoid the remote driver internally rather than via a public API flag
Peter Krempa (9): virConnectOpenInternal: Avoid double free() when alias is an invalid URI virConnectOpenInternal: Switch to automatic memory cleanup virConnectOpenInternal: Remove 'failed' label remote: remoteOpenConn: Use virConnectOpenAuth instead of virConnectOpen(ReadOnly) remoteConnectOpen: Refactor cleanup remote: doRemoteOpen: Automatically clean up 'priv' lxc: Remove unneeded forward declaration of 'lxcStateInitialize' virStateInitialize: Propagate whether running in monolithic daemon mode to stateful driver init remote: Don't attempt remote connection from libvirtd
src/bhyve/bhyve_driver.c | 1 + src/ch/ch_driver.c | 1 + src/driver-state.h | 1 + src/interface/interface_backend_netcf.c | 1 + src/interface/interface_backend_udev.c | 1 + src/libvirt.c | 67 +++++++++++-------------- src/libvirt_internal.h | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 5 +- src/network/bridge_driver.c | 1 + src/node_device/node_device_udev.c | 1 + src/nwfilter/nwfilter_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_daemon.c | 6 +++ src/remote/remote_daemon_dispatch.c | 7 ++- src/remote/remote_driver.c | 59 +++++++++++----------- src/secret/secret_driver.c | 1 + src/storage/storage_driver.c | 1 + src/vz/vz_driver.c | 1 + 19 files changed, 83 insertions(+), 75 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa