[libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure

[Eric Blake]

ping

On 10/17/18 8:45 PM, Eric Blake wrote:
> If qemuDomainSnapshotDiscard() fails for any reason (rare,
> but possible with an ill-timed ENOMEM or if
> qemuDomainSnapshotForEachQcow2() has problems talking to the
> qemu guest monitor), then an attempt to retry the snapshot
> deletion API will crash because we didn't undo the effects
> of virDomainSnapshotDropParent() temporarily rearranging the
> internal list structures, and the second attempt to drop
> parents will dereference NULL.  Fix it by instead noting that
> there are only two callers to qemuDomainSnapshotDiscard(),
> and only one of the two callers wants the parent to be updated;
> thus we can move the call to virDomainSnapshotDropParent()
> into a code path that only gets executed on success.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> 
> ---
> v2: avoid use-after-free
> ---
>   src/qemu/qemu_domain.c | 6 ++++--
>   src/qemu/qemu_driver.c | 1 -
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f00f1b3fdb..dd67be5e2a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8246,7 +8246,7 @@ int
>   qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>                             virDomainObjPtr vm,
>                             virDomainSnapshotObjPtr snap,
> -                          bool update_current,
> +                          bool update_parent,
>                             bool metadata_only)
>   {
>       char *snapFile = NULL;
> @@ -8275,7 +8275,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>           goto cleanup;
> 
>       if (snap == vm->current_snapshot) {
> -        if (update_current && snap->def->parent) {
> +        if (update_parent && snap->def->parent) {
>               parentsnap = virDomainSnapshotFindByName(vm->snapshots,
>                                                        snap->def->parent);
>               if (!parentsnap) {
> @@ -8298,6 +8298,8 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> 
>       if (unlink(snapFile) < 0)
>           VIR_WARN("Failed to unlink %s", snapFile);
> +    if (update_parent)
> +        virDomainSnapshotDropParent(snap);
>       virDomainSnapshotObjListRemove(vm->snapshots, snap);
> 
>       ret = 0;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a52e2495d5..9f71641dfa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16526,7 +16526,6 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>           snap->first_child = NULL;
>           ret = 0;
>       } else {
> -        virDomainSnapshotDropParent(snap);
>           ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
>       }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org