5:56 p.m.
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(a)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
4:05 p.m.
New subject: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not
found in VM definition
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(a)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
6:10 p.m.
New subject: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not
found in VM definition
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(a)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
>
6:19 p.m.
New subject: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not
found in VM definition
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(a)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(a)redhat.com>
79
days inactive
140
days old
3 comments
3 participants
participants (3)
-
kaihuan -
Martin Kletzander -
Peter Krempa