[libvirt] [PATCH] qemu: don't munge user input during block commit
Eric Blake
eblake at redhat.com
Thu Mar 6 23:47:16 UTC 2014
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 <eblake at redhat.com>
---
I was _hoping_ that this would solve the mentioned bugzilla. But
even with this applied, qemu still ended up writing an absolute
backing file name into the active file in the backing chain, so that
bug has been reassigned back to qemu - it probably has to do with
the fact that libvirt always spawns qemu with -drive pointing to
an absolute name, which is unrelated to what this patch fixes.
Therefore, I'm a little bit hesitant to apply this patch, but
wanted to post it for review anyway.
src/qemu/qemu_driver.c | 11 ++++++++---
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c | 2 +-
src/qemu/qemu_monitor_json.h | 5 +++--
5 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9aad2dc..31adf78 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15383,10 +15383,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 behaves 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 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 a2769db..e4707b7 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 eabf000..7bb465b 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 ee3ae15..40d6d1a 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 a93c51e..ef71588 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,
--
1.8.5.3
More information about the libvir-list
mailing list