qemuDomainDiskByName() can return a NULL pointer on failure, but this
returned value in qemuSnapshotDeleteValidate is not checked.
It will make libvirtd crash.

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 5b3aadcbf0..52312b4a7b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -4235,8 +4235,11 @@ 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)))
+                return -1;
+
+            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom,
snapDisk->name)))
+                return -1;

             if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
                 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,

Typo in shortlog (cransh); use:


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

On Thu, Nov 21, 2024 at 23:36:14 +0800, jungle man wrote:
> qemuDomainDiskByName() can return a NULL pointer on failure, but this
> returned value in qemuSnapshotDeleteValidate is not checked.
> It will make libvirtd crash.

Couple problems:
 1) your patch got corrupted when sending
 2) your patch is missing the certification that it is in compliance
    with the Developer Certificate of Origin (missing sign-off):

     https://www.libvirt.org/hacking.html#developer-certificate-of-origin

> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 5b3aadcbf0..52312b4a7b 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -4235,8 +4235,11 @@ 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)))
> +                return -1;
> +
> +            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom,
> snapDisk->name)))
> +                return -1;

 3) While these functions do report their own error and we generallu use
 that, I think in this case it'll be confusing as it will not tell the
 user in which config the disk is missing. I think we need specific
 errors in this case. e.g.:

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

and for the other error something like:

  _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),

Thank you.

I will check it out.and resend in a new email




On Wed, Nov 27, 2024 at 5:49 PM Peter Krempa <pkrempa@redhat.com> wrote:

>
> Typo in shortlog (cransh); use:
>
>
> qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM
> definition
>
> On Thu, Nov 21, 2024 at 23:36:14 +0800, jungle man wrote:
> > qemuDomainDiskByName() can return a NULL pointer on failure, but this
> > returned value in qemuSnapshotDeleteValidate is not checked.
> > It will make libvirtd crash.
>
> Couple problems:
>  1) your patch got corrupted when sending
>  2) your patch is missing the certification that it is in compliance
>     with the Developer Certificate of Origin (missing sign-off):
>
>      https://www.libvirt.org/hacking.html#developer-certificate-of-origin
>
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index 5b3aadcbf0..52312b4a7b 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -4235,8 +4235,11 @@ 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)))
> > +                return -1;
> > +
> > +            if (!(disk = qemuDomainDiskByName(snapdef->parent.dom,
> > snapDisk->name)))
> > +                return -1;
>
>  3) While these functions do report their own error and we generallu use
>  that, I think in this case it'll be confusing as it will not tell the
>  user in which config the disk is missing. I think we need specific
>  errors in this case. e.g.:
>
>  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;
>  }
>
> and for the other error something like:
>
>   _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM
> definition of the deleted snapshot"),
>
>