[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]