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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list