10:15 a.m.
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
10:15 a.m.
New subject: [PATCH 1/6] virConnectOpenInternal: Avoid double free() when alias is an invalid URI
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(a)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
10:15 a.m.
New subject: [PATCH 2/6] virConnectOpenInternal: Switch to automatic memory cleanup
Use automatic cleanup for 'ret' and 'uristr'.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:15 a.m.
New subject: [PATCH 3/6] virConnectOpenInternal: Remove 'failed' label
Jumping to the label would just return NULL.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:15 a.m.
New subject: [PATCH 4/6] remote: remoteOpenConn: Use virConnectOpenAuth instead of virConnectOpen(ReadOnly)
virConnectOpenAuth provides an unified interface with using 'flags' to
select the proper mode.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:15 a.m.
New subject: [PATCH 5/6] Introduce VIR_CONNECT_NO_REMOTE
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(a)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
10:16 a.m.
New subject: [PATCH 6/6] remote: Don't attempt remote connection from libvirtd
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(a)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
10:25 a.m.
New subject: [PATCH 6/6] remote: Don't attempt remote connection from libvirtd
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(a)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 :|
10:53 a.m.
New subject: [PATCH 6/6] remote: Don't attempt remote connection from libvirtd
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?
11:04 a.m.
New subject: [PATCH 6/6] remote: Don't attempt remote connection from libvirtd
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 :|
798
days inactive
798
days old
9 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Peter Krempa