[libvirt] [PATCHv4 23/51] snapshot: add qemu snapshot redefine support

[Eric Blake]

On 09/01/2011 10:25 PM, Eric Blake wrote:
> Redefining a qemu snapshot requires a bit of a tweak to the common
> snapshot parsing code, but the end result is quite nice.
>
> +    if (flags&  VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
> +        virDomainSnapshotObjPtr prior = NULL;
> +
> +        prior = virDomainSnapshotFindByName(&vm->snapshots, def->name);
> +        if (prior) {
> +            /* XXX Ensure ABI compatibility before replacing anything.  */
> +            virDomainSnapshotObjListRemove(&vm->snapshots, prior);
> +        }
> +    }

This validation is pretty weak, and can be easily exploited to cause 
libvirtd to infloop.  I'm strengthening it by squashing in the following 
on this patch (plus later patches, when snapshot->def->dom is added, 
will add ABI compatibility checking).

[Technically, a user can still cause libvirtd infloops by messing 
directly with files in /var/lib/libvirt/qemu/snapshot/dom/*.xml, but 
those files are supposed to be protected, and touching them outside of 
libvirt API is already in unsupported territory - our goal only has to 
be that the API can't be abused to get into bad state, not to protect 
ourselves from someone clobbering our internal directories.]

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index f862b81..de13584 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -8664,12 +8664,57 @@ static virDomainSnapshotPtr 
qemuDomainSnapshotCreateXML(virDomainPtr domain,
          goto cleanup;

      if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
-        virDomainSnapshotObjPtr prior = NULL;
+        virDomainSnapshotObjPtr other = NULL;

-        prior = virDomainSnapshotFindByName(&vm->snapshots, def->name);
-        if (prior) {
+        /* Prevent circular chains */
+        if (def->parent) {
+            if (STREQ(def->name, def->parent)) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("cannot set snapshot %s as its own 
parent"),
+                                def->name);
+                goto cleanup;
+            }
+            other = virDomainSnapshotFindByName(&vm->snapshots, 
def->parent);
+            if (!other) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("parent %s for snapshot %s not found"),
+                                def->parent, def->name);
+                goto cleanup;
+            }
+            while (other->def->parent) {
+                if (STREQ(other->def->parent, def->name)) {
+                    qemuReportError(VIR_ERR_INVALID_ARG,
+                                    _("parent %s would create cycle to 
%s"),
+                                    other->def->name, def->name);
+                    goto cleanup;
+                }
+                other = virDomainSnapshotFindByName(&vm->snapshots,
+                                                    other->def->parent);
+                if (!other) {
+                    VIR_WARN("snapshots are inconsistent for %s",
+                             vm->def->name);
+                    break;
+                }
+            }
+        }
+
+        /* Check that any replacement is compatible */
+        other = virDomainSnapshotFindByName(&vm->snapshots, def->name);
+        if (other) {
+            if (other == vm->current_snapshot)
+                def->current == true;
+            if ((other->def->state == VIR_DOMAIN_RUNNING ||
+                 other->def->state == VIR_DOMAIN_PAUSED) !=
+                (def->state == VIR_DOMAIN_RUNNING ||
+                 def->state == VIR_DOMAIN_PAUSED)) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("cannot change between online and 
offline "
+                                  "snapshot state in snapshot %s"),
+                                def->name);
+                goto cleanup;
+            }
              /* XXX Ensure ABI compatibility before replacing anything.  */
-            virDomainSnapshotObjListRemove(&vm->snapshots, prior);
+            virDomainSnapshotObjListRemove(&vm->snapshots, other);
          }
      }

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org