From: Eric Blake Date: Thu, 6 Mar 2014 23:33:16 +0000 (-0700) Subject: qemu: don't munge user input during block commit X-Git-Tag: v1.2.3-rc1~294 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=359f4b11a66f77dcdfec0cf099b5353602c6de46;p=thirdparty%2Flibvirt.git qemu: don't munge user input during block commit While investigating https://bugzilla.redhat.com/show_bug.cgi?id=1061827 I noticed that we pass user input unscathed for block-pull, but always pass a canonical absolute name through for block-commit. [Note that we probably _ought_ to validate that the user's request for block-pull actually matches the backing chain, the way we already do for block-commit - but that's a separate issue. Further note that the ability to pass user input through unscathed allows backdoors such as specifying a backing image that is a network URI such as a gluster disk, instead of forcing things to the local file system; which is an area still under active investigation on whether libvirt needs to behave differently for network disks.] Since qemu may write the name that the user passed in as the backing file, a user may have a reason to want a relative file name passed through to qemu, and always munging things to absolute prevents that. Put another way, if you have the backing chain: [A] <- [B(back=./A)] <- [C(back=./B)] and commit B into A (virsh blockcommit $dom vda --base A --top B), the metadata of C will have to be re-written. But should it be rewritten as [C(back=./A)] or as [C(back=/path/to/A)]? Still up in the air is whether qemu's decision should be based on whether B and/or C had relative paths, or on whether the --base and/or --top arguments to the command were relative paths; but if we always pass a canonical name, we've prevented the spelling of the command arguments from being part of the hueristics that qemu uses. I also audited the code, and verified that we never call qemuMonitorBlockCommit() with a NULL base, either before or after the change to qemu_driver.c. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Preserve user's spelling, since absolute vs. relative matters to qemu. * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): Base is never null. * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit): Likewise. Signed-off-by: Eric Blake --- diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa7b129b3b..b167949d9e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15437,10 +15437,15 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; - /* Start the commit operation. */ + /* Start the commit operation. Pass the user's original spelling, + * if any, through to qemu, since qemu may behave differently + * depending on whether the input was specified as relative or + * absolute (that is, our absolute top_canon may do the wrong + * thing if the user specified a relative name). */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockCommit(priv->mon, device, top_canon, base_canon, - bandwidth); + ret = qemuMonitorBlockCommit(priv->mon, device, + top ? top : top_canon, + base ? base : base_canon, bandwidth); qemuDomainObjExitMonitor(driver, vm); endjob: diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a2769dbe2b..e4707b7c05 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1,7 +1,7 @@ /* * qemu_monitor.c: interaction with QEMU monitor console * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -3188,7 +3188,7 @@ qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, unsigned long long speed; VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", - mon, device, NULLSTR(top), NULLSTR(base), bandwidth); + mon, device, top, base, bandwidth); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index eabf000969..7bb465bea9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -647,7 +647,8 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *top, const char *base, unsigned long bandwidth) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7d6b88b326..dbc04f68d2 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3307,7 +3307,7 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, "s:device", device, "U:speed", speed, "s:top", top, - base ? "s:base" : NULL, base, + "s:base", base, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a93c51eeb0..ef71588269 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -1,7 +1,7 @@ /* * qemu_monitor_json.h: interaction with QEMU monitor console * - * Copyright (C) 2006-2009, 2011-2013 Red Hat, Inc. + * Copyright (C) 2006-2009, 2011-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -258,7 +258,8 @@ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *top, const char *base, unsigned long long bandwidth) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str,