[libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
Martin Kletzander
mkletzan at redhat.com
Fri May 12 11:28:22 UTC 2017
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 at 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...
>
>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.
>Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170512/c8ab79bb/attachment-0001.sig>
More information about the libvir-list
mailing list