[PATCH] qemu:qemu_snapshot: Fix a libvirtd cransh when delete snapshot
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"),
participants (2)
-
jungle man -
Peter Krempa