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

On 09/01/2011 10:25 PM, Eric Blake wrote: 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org