4:27 a.m.
On Thu, May 11, 2017 at 05:49:54PM +0200, Pavel Hrdina wrote:
Currently we consider all UNIX paths with specific prefix as
generated
by libvirt, but that's a wrong assumption. Let's make the detection
<rant tldr="sigh">
That assumption is pretty OK from my POV, any name that's not generated
by libvirt under that path can collide with anything. Only libvirt paths
can be guaranteed not to collide. That's where generated paths go and
users or mgmt apps should query that information from the running XML.
That's also the only way we can guarantee access works.
But hey, why not make our codebase bigger and more complex, right...
</rant>
better by actually checking whether the whole path matches one of the
paths that we generate or generated in the past.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
.../qemuxml2argv-channel-unix-gen-path1.xml | 17 +++++
.../qemuxml2argv-channel-unix-gen-path2.xml | 17 +++++
.../qemuxml2argv-channel-unix-gen-path3.xml | 17 +++++
.../qemuxml2argv-channel-unix-user-path.xml | 17 +++++
.../qemuxml2xmlout-channel-unix-gen-path1.xml | 32 +++++++++
.../qemuxml2xmlout-channel-unix-gen-path2.xml | 32 +++++++++
.../qemuxml2xmlout-channel-unix-gen-path3.xml | 32 +++++++++
.../qemuxml2xmlout-channel-unix-user-path.xml | 33 +++++++++
tests/qemuxml2xmltest.c | 5 ++
10 files changed, 271 insertions(+), 9 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc02c801e1..99bfd8f320 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3154,24 +3154,84 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
/*
- * Clear auto generated unix socket path, i.e., the one which starts with our
- * channel directory.
+ * Clear auto generated unix socket paths:
+ *
+ * libvirt 1.2.18 and older:
+ * {cfg->channelTargetDir}/{dom-name}.{target-name}
+ *
+ * libvirt 1.2.19 - 1.3.2:
+ * {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
+ *
+ * libvirt 1.3.3 and newer:
+ * {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
+ *
Looks like we could just clean the last one. People with previous
versions might rely on the generated paths already...
We are clearing the path since 1.3.1. Might be worth putting it in the
commit message.
+ * The unix socket path was stored in config XML until libvirt
1.3.0.
+ * If someone specifies the same path as we generate, they shouldn't do it.
+ *
+ * This function clears the path for migration as well, so we need to clear
+ * the path event if we are not storing it in the XML.
*/
-static void
+static int
qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
virQEMUDriverPtr driver)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ char *path;
+ char *prefix = NULL;
+ int prefixLen;
+ int ret = -1;
+ int rv;
- if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
- chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
- chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
- chr->source->data.nix.path &&
- STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
- VIR_FREE(chr->source->data.nix.path);
+ if (chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
+ chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
+ chr->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
+ !chr->source->data.nix.path) {
+ ret = 0;
+ goto cleanup;
}
+ if (!STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ path = chr->source->data.nix.path + strlen(cfg->channelTargetDir);
+ prefixLen = strlen(path) - strlen(chr->target.name);
+
+ if (STRNEQ(path + prefixLen, chr->target.name)) {
If this can happen, that means it can eventually (in very rare
circumstances) segfault if the target name is very long.
I hope target names cannot have international characters in them,
otherwise it's even worse.
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!VIR_STRNDUP(prefix, path, prefixLen))
+ goto cleanup;
+
+ /* Now we've isolated the middle part of the path by removing the
+ * cfg->channelTargetDir part from the beginning and chr->target.name
+ * from the end. The middle part is the one that changed in the past
+ * and the only part that we need to try to match with. */
+
+#define VIR_CLEAR_CHR_DEF_PATH(regex) \
+ if ((rv = virStringMatch(prefix, regex)) < 0) \
+ goto cleanup; \
+ \
+ if (rv == 0) { \
+ VIR_FREE(chr->source->data.nix.path); \
+ ret = 0; \
+ goto cleanup; \
+ }
+
+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
+
Just check for ^{cfg->channelTargetDir}/[^/]+[./]qemu/{chr->targetName}
And make sure you escape stuff in squiggly brackets. That should work
and be easier. Also don't forget adapting it to the boolean return
value of virStringMatch