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@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);
     }

  endjob:
-- 
2.17.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list