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@xxxxxxxxx>
> > ---
> > 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
> >
