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"),