[PATCH 0/6] Fix 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. Peter Krempa (6): 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) Introduce VIR_CONNECT_NO_REMOTE remote: Don't attempt remote connection from libvirtd include/libvirt/libvirt-host.h | 2 + src/libvirt.c | 98 ++++++++++++++--------------- src/remote/remote_daemon_dispatch.c | 15 +++-- 3 files changed, 62 insertions(+), 53 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
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
Introduce a new virConnectFlags flag that will allow to avoid the use of the 'remote' driver. While this flag itself is not useful for users, in case when monolithic libvirtd is in use we want to avoid further delegation of connection, which can happen if libvirtd is not compiled with the requested driver. Add and implement the new flag. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-host.h | 2 ++ src/libvirt.c | 38 +++++++++++++++++++++------------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h index 3112f2b676..463b3f78af 100644 --- a/include/libvirt/libvirt-host.h +++ b/include/libvirt/libvirt-host.h @@ -602,6 +602,8 @@ int virNodeGetSEVInfo (virConnectPtr conn, typedef enum { VIR_CONNECT_RO = (1 << 0), /* A readonly connection (Since: 0.4.1) */ VIR_CONNECT_NO_ALIASES = (1 << 1), /* Don't try to resolve URI aliases (Since: 0.9.7) */ + VIR_CONNECT_NO_REMOTE = (1 << 2), /* Avoid use of the remote driver. Users + should not use this flag. (Since: 8.8.0) */ } virConnectFlags; /** diff --git a/src/libvirt.c b/src/libvirt.c index d5ae68d16f..5312265619 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -886,6 +886,9 @@ virConnectOpenInternal(const char *name, g_autoptr(virConf) conf = NULL; g_autofree char *uristr = NULL; bool embed = false; + bool no_remote = flags & VIR_CONNECT_NO_REMOTE; + + flags &= ~VIR_CONNECT_NO_REMOTE; ret = virGetConnect(); if (ret == NULL) @@ -1036,26 +1039,33 @@ virConnectOpenInternal(const char *name, * driver then report a useful error, instead of a cryptic one about * not being able to connect to libvirtd or not being able to find * certificates. */ - if (STREQ(virConnectDriverTab[i]->hypervisorDriver->name, "remote") && - ret->uri != NULL && - ( + if (STREQ(virConnectDriverTab[i]->hypervisorDriver->name, "remote")) { + + if (ret->uri != NULL && + ( #ifndef WITH_ESX - STRCASEEQ(ret->uri->scheme, "vpx") || - STRCASEEQ(ret->uri->scheme, "esx") || - STRCASEEQ(ret->uri->scheme, "gsx") || + STRCASEEQ(ret->uri->scheme, "vpx") || + STRCASEEQ(ret->uri->scheme, "esx") || + STRCASEEQ(ret->uri->scheme, "gsx") || #endif #ifndef WITH_HYPERV - STRCASEEQ(ret->uri->scheme, "hyperv") || + STRCASEEQ(ret->uri->scheme, "hyperv") || #endif #ifndef WITH_VZ - STRCASEEQ(ret->uri->scheme, "parallels") || + STRCASEEQ(ret->uri->scheme, "parallels") || #endif - false)) { - virReportErrorHelper(VIR_FROM_NONE, VIR_ERR_CONFIG_UNSUPPORTED, - __FILE__, __FUNCTION__, __LINE__, - _("libvirt was built without the '%s' driver"), - ret->uri->scheme); - return NULL; + false)) { + virReportErrorHelper(VIR_FROM_NONE, VIR_ERR_CONFIG_UNSUPPORTED, + __FILE__, __FUNCTION__, __LINE__, + _("libvirt was built without the '%s' driver"), + ret->uri->scheme); + return NULL; + } + + if (no_remote) { + VIR_DEBUG("skipping 'remote' per VIR_CONNECT_NO_REMOTE"); + continue; + } } VIR_DEBUG("trying driver %zu (%s) ...", -- 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. Use the new flag VIR_CONNECT_NO_REMOTE and pass it from libvirtd to the opening function to avoid delegation in this specific case. After this patch the above attempt produces: $ 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_daemon_dispatch.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 061e0f7811..d7c473d2a1 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1798,6 +1798,14 @@ remoteOpenConn(const char *uri, if (*conn) return 0; +#ifdef LIBVIRTD + /* When libvirtd is in use we need to avoid any further delegation of the + * connection, which can be attempted in cases when the appropriate + * connection driver was not compiled in. In such case a wrong error message + * would be reported. */ + connectFlags |= VIR_CONNECT_NO_REMOTE; +#endif /* LIBVIRTD */ + if (!uri) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); return -1; -- 2.37.1
On Thu, Sep 08, 2022 at 05:16:00PM +0200, Peter Krempa wrote:
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.
Use the new flag VIR_CONNECT_NO_REMOTE and pass it from libvirtd to the opening function to avoid delegation in this specific case. After this patch the above attempt produces:
$ 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_daemon_dispatch.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 061e0f7811..d7c473d2a1 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1798,6 +1798,14 @@ remoteOpenConn(const char *uri, if (*conn) return 0;
+#ifdef LIBVIRTD + /* When libvirtd is in use we need to avoid any further delegation of the + * connection, which can be attempted in cases when the appropriate + * connection driver was not compiled in. In such case a wrong error message + * would be reported. */ + connectFlags |= VIR_CONNECT_NO_REMOTE; +#endif /* LIBVIRTD */
This flag shouldn't be required in the public API. THis code and the remote driver are both in the same process, so it ought to be possible to block this using the 'inside_daemon' flag that we already use for similar reasons in the remote driver. This just feels like an edge case that we missed in our use of 'inside_daemon' With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Sep 08, 2022 at 16:25:39 +0100, Daniel P. Berrangé wrote:
On Thu, Sep 08, 2022 at 05:16:00PM +0200, Peter Krempa wrote:
[...]
+#ifdef LIBVIRTD + /* When libvirtd is in use we need to avoid any further delegation of the + * connection, which can be attempted in cases when the appropriate + * connection driver was not compiled in. In such case a wrong error message + * would be reported. */ + connectFlags |= VIR_CONNECT_NO_REMOTE; +#endif /* LIBVIRTD */
This flag shouldn't be required in the public API. THis code and the remote driver are both in the same process, so it ought to be possible to block this using the 'inside_daemon' flag that we already use for similar reasons in the remote driver. This just feels like an edge case that we missed in our use of 'inside_daemon'
Hmm, yeah, it should be possible to achieve the same behaviour by adding a conditionally compiled block to the 'inside_daemon' block in remoteConnectOpen which refuses to open the connection if the daemon is 'libvirtd'. IIUC other daemons do need to allow delegation, right?
On Thu, Sep 08, 2022 at 05:53:05PM +0200, Peter Krempa wrote:
On Thu, Sep 08, 2022 at 16:25:39 +0100, Daniel P. Berrangé wrote:
On Thu, Sep 08, 2022 at 05:16:00PM +0200, Peter Krempa wrote:
[...]
+#ifdef LIBVIRTD + /* When libvirtd is in use we need to avoid any further delegation of the + * connection, which can be attempted in cases when the appropriate + * connection driver was not compiled in. In such case a wrong error message + * would be reported. */ + connectFlags |= VIR_CONNECT_NO_REMOTE; +#endif /* LIBVIRTD */
This flag shouldn't be required in the public API. THis code and the remote driver are both in the same process, so it ought to be possible to block this using the 'inside_daemon' flag that we already use for similar reasons in the remote driver. This just feels like an edge case that we missed in our use of 'inside_daemon'
Hmm, yeah, it should be possible to achieve the same behaviour by adding a conditionally compiled block to the 'inside_daemon' block in remoteConnectOpen which refuses to open the connection if the daemon is 'libvirtd'.
IIUC other daemons do need to allow delegation, right?
Yes, for example, virtproxy needs to be able to contact all other daemons. virtqemud, virtstoraged, need to talk to virtsecretd. Even libvirtd needs to talk to other daemons, but, *ONLY* if a non-NULL server component is present. So I believe we need to block libvirtd when server == NULL only. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa