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

[Eric Blake]

On 10/17/18 7:30 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 only
> rearranging the internal list on success.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> Found by accident, since I copied this code to my checkpoint
> code and hit the segfault there.  This one has been latent
> for years, but is not a security bug since there is no escalation
> if you already have sufficient privilege to delete snapshots.
> 
>   src/qemu/qemu_driver.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3f143e91c1..9a3f2a090d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16699,8 +16699,9 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>           snap->first_child = NULL;
>           ret = 0;
>       } else {
> -        virDomainSnapshotDropParent(snap);
>           ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
> +        if (ret == 0)
> +            virDomainSnapshotDropParent(snap);

NACK; this creates a use-after-free scenario, since snap is freed by 
qemuDomainSnapshotDiscard on success.  I'll have to come up with 
something different.

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