1. Patchew
  2. Libvirt
  3. View series

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

Eric Blake posted 1 patch 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181018014535.259884-1-eblake@redhat.com
Test syntax-check passed
src/qemu/qemu_domain.c | 6 ++++--
src/qemu/qemu_driver.c | 1 -
2 files changed, 4 insertions(+), 3 deletions(-)
[libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure
Posted by Eric Blake 5 years, 6 months ago 
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@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);
     }

-- 
2.17.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure
Posted by Eric Blake 5 years, 5 months ago 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure
Posted by Michal Privoznik 5 years, 5 months ago 
On 10/18/2018 03:45 AM, 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@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(-)

ACK

Michal

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.