Re: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt

On Fri, May 12, 2017 at 12:02:24PM +0200, Pavel Hrdina wrote: >On Fri, May 12, 2017 at 11:27:36AM +0200, Martin Kletzander wrote: >> 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... > >Yes, I agree, but we should've documented it and possible check the path >provided by user and error out if it matches the prefix of our internal >paths and we shouldn't have put the path into config XML. > >A lot of code in libvirt is there just to make sure that every bad >design or decision still works :). > And even more code that is left out so that people can shoot themselves in their feet if they want to. And precisely for the same reason. >> </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&gt; >> >--- >> > 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... > >I don't think so. Currently we clear all of them because we match only >the prefix, this relaxes the logic only to the paths that we've actually >generated. > >This code is also used to clear the path for migration so removing only >the latest format would break migration of guests that would use the old >format. You can have a running guest, you update libvirt and restart >the daemon and after that you try to migrate and it suddenly fails, that >would be a regression. It would also break incoming migration from >libvirt that doesn't clear the path before sending an XML. > >We need to live with the fact, that the generated-path wasn't somehow >distinguished from path provided by user and because of that we need >to have an extra code to handle it. > I know, I wrote the code that cleans it up. I just meant to say not many people will upgrade from version older than 1.3.1. Also it's not a requirement, just another 'sigh'. >> 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. > >Nice catch, I'll fix it. > >> >+ 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 > >That's exactly what I wanted to avoid, escaping the path where we can >simply use STRPREFIX and STRNEQ. > Which you can't. And you are constructing and doing the regexp match three times. Moreover you don't need to escape much. Just "^$.|?*+()[]{}\" and all those with just a backslash. You can add few more checks there, but just doing one escape and one match is way more readable.