1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition

kaihuan posted 1 patch 3 months, 2 weeks ago 
src/qemu/qemu_snapshot.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
[PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
Posted by kaihuan 3 months, 2 weeks ago 
qemuDomainDiskByName() can return a NULL pointer on failure.
But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash.

Signed-off-by: kaihuan <jungleman759@gmail.com>
---
 src/qemu/qemu_snapshot.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 18b2e478f6..bcbd913073 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
             virDomainDiskDef *vmdisk = NULL;
             virDomainDiskDef *disk = NULL;
 
-            vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
-            disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
+            if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {
+                virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
+                            snapDisk->name, snap->def->name);
+                return -1;
+            }
+
+            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) {
+                virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
+                            snapDisk->name, snap->def->name);
+                return -1;
+            }
 
             if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
                 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-- 
2.33.1.windows.1
Re: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
Posted by Peter Krempa 1 month, 2 weeks ago 
On Fri, Nov 29, 2024 at 22:56:45 +0800, kaihuan wrote:
> qemuDomainDiskByName() can return a NULL pointer on failure.
> But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash.
> 
> Signed-off-by: kaihuan <jungleman759@gmail.com>
> ---
>  src/qemu/qemu_snapshot.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 18b2e478f6..bcbd913073 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
>              virDomainDiskDef *vmdisk = NULL;
>              virDomainDiskDef *disk = NULL;
>  
> -            vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> -            disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
> +            if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
> +                            snapDisk->name, snap->def->name);
> +                return -1;
> +            }
> +
> +            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
> +                            snapDisk->name, snap->def->name);
> +                return -1;
> +            }
>  
>              if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
>                  virReportError(VIR_ERR_OPERATION_UNSUPPORTED,

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Re: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
Posted by Martin Kletzander 1 month, 3 weeks ago 
On Fri, Nov 29, 2024 at 10:56:45PM +0800, kaihuan wrote:
>qemuDomainDiskByName() can return a NULL pointer on failure.
>But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash.
>

Hi, looking through old unreviewed patches I found this one.  Sorry for
the wait.  Can you explain whether you found out how to trigger that?
This looks like there is some other issue that causes a snapshot having
a disk that is not in the domain.  Does that happen when you want to
delete a snapshot after unplugging a disk?

>Signed-off-by: kaihuan <jungleman759@gmail.com>
>---
> src/qemu/qemu_snapshot.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
>index 18b2e478f6..bcbd913073 100644
>--- a/src/qemu/qemu_snapshot.c
>+++ b/src/qemu/qemu_snapshot.c
>@@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
>             virDomainDiskDef *vmdisk = NULL;
>             virDomainDiskDef *disk = NULL;
>
>-            vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
>-            disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);

The function calls already report an error when the disk is not found,
so there is not need to rewrite that error in my opinion.

>+            if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {
>+                virReportError(VIR_ERR_OPERATION_FAILED,
>+                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
>+                            snapDisk->name, snap->def->name);
>+                return -1;
>+            }
>+
>+            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) {
>+                virReportError(VIR_ERR_OPERATION_FAILED,
>+                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
>+                            snapDisk->name, snap->def->name);
>+                return -1;
>+            }
>
>             if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
>                 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>-- 
>2.33.1.windows.1
>
Re: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
Posted by Peter Krempa 1 month, 2 weeks ago 
On Wed, Jan 22, 2025 at 14:05:18 +0100, Martin Kletzander wrote:
> On Fri, Nov 29, 2024 at 10:56:45PM +0800, kaihuan wrote:
> > qemuDomainDiskByName() can return a NULL pointer on failure.
> > But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash.
> > 
> 
> Hi, looking through old unreviewed patches I found this one.  Sorry for
> the wait.  Can you explain whether you found out how to trigger that?
> This looks like there is some other issue that causes a snapshot having
> a disk that is not in the domain.  Does that happen when you want to
> delete a snapshot after unplugging a disk?
> 
> > Signed-off-by: kaihuan <jungleman759@gmail.com>
> > ---
> > src/qemu/qemu_snapshot.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index 18b2e478f6..bcbd913073 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
> >             virDomainDiskDef *vmdisk = NULL;
> >             virDomainDiskDef *disk = NULL;
> > 
> > -            vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> > -            disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
> 
> The function calls already report an error when the disk is not found,
> so there is not need to rewrite that error in my opinion.

Normally I'd agree, but here ...

> 
> > +            if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {

you look it up in one definition ...

> > +                virReportError(VIR_ERR_OPERATION_FAILED,
> > +                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
> > +                            snapDisk->name, snap->def->name);
> > +                return -1;
> > +            }
> > +
> > +            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) {

and directly after look it up in another definition with exactly the
same function.

With the default error it'll be very hard to figure out what's going on
so in this case we should indeed overwrite the error as an exception to
the rule.

> > +                virReportError(VIR_ERR_OPERATION_FAILED,
> > +                            _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
> > +                            snapDisk->name, snap->def->name);
> > +                return -1;
> > +            }
> > 
> >             if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
> >                 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > -- 
> > 2.33.1.windows.1
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.