- Devel - libvirt List Archives

Re: [PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
by jungleman759 22 Apr '25

22 Apr '25

Hi Thanks for following up, and sorry for the delay in getting back to you. You're right to suspect the issue might be related to device changes. Here’s how the crash can be triggered: The VM initially uses a SATA controller, with a disk defined as: xml 复制编辑 <controller type="scsi" index="0" model="lsilogic"></controller> <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/Testguest.qcow2'/> <target dev='sda' bus='sata'/> </disk> A snapshot is created at this point — which records the disk as sda. Later, the VM is reconfigured to use a virtio controller, and the disk is now assigned as vda. When the VM is running and try to delete the snapshot, the snapshot code still expects to find a disk named sda in the current VM definition. Because of this mismatch, qemuDomainDiskByName() returns NULL, and the crash occurs when the result is used without a null check. This can easily happen during controller or disk bus reconfiguration between snapshot and deletion. The patch adds sanity checks to ensure we don’t dereference a null pointer in this situation. Let me know if you’d like me to adjust the wording in the error messages or add a reproducer for automated testing. Best regards, kaihuan Martin Kletzander <mkletzan(a)redhat.com> 于2025年1月22日周三 21:05写道： 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 >

1 0

[PATCH 0/3] remote: improve missing socket diagnostics
by Daniel P. Berrangé 17 Apr '25

17 Apr '25

Daniel P. Berrangé (3): remote: improve error message when no URI is set kbase: update docs to account for changed error message remote: expand some debug messages for socket detection docs/kbase/failed_connection_after_install.rst | 10 ++++++++++ src/remote/remote_sockets.c | 9 +++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) -- 2.48.1

2 5

[PATCH 0/3] Fix CI, build, and a typo
by Martin Kletzander 16 Apr '25

16 Apr '25

Each in its respecive commit, obviously. Martin Kletzander (3): nwfilter: Fix erroneous pointer passing to g_clear_pointer python: Do not explicitly state variables are global when only read qemu_rdp: Fix a typo existance -> existence scripts/apibuild.py | 6 ------ src/nwfilter/nwfilter_gentech_driver.c | 4 ++-- src/qemu/qemu_rdp.c | 2 +- tools/virt-qemu-qmp-proxy | 1 - 4 files changed, 3 insertions(+), 10 deletions(-) -- 2.49.0

2 4

[PATCH] virbitmap: Change return type of virBitmapToData to void
by Alexander Kuznetsov 16 Apr '25

16 Apr '25

This function return value is invariant since e59b8f9, so change its type and remove all dependent checks. Found by Linux Verification Center (linuxtesting.org) with Svace. Signed-off-by: Artem Chernyshev <artem.chernyshev(a)red-soft.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam(a)altlinux.org> --- src/ch/ch_monitor.c | 5 +---- src/hypervisor/domain_driver.c | 4 +--- src/qemu/qemu_driver.c | 3 +-- src/util/virbitmap.c | 6 +----- src/util/virbitmap.h | 2 +- src/util/virhostcpu.c | 4 ++-- tests/virbitmaptest.c | 3 +-- tools/virsh-completer-domain.c | 6 ++---- tools/virsh-domain.c | 3 +-- 9 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 0ba927a194..b772eec8a2 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -1196,10 +1196,7 @@ virCHMonitorGetIOThreads(virCHMonitor *mon, if (!(map = virProcessGetAffinity(iothreadinfo->iothread_id))) goto error; - if (virBitmapToData(map, &(iothreadinfo->cpumap), - &(iothreadinfo->cpumaplen)) < 0) { - goto error; - } + virBitmapToData(map, &(iothreadinfo->cpumap), &(iothreadinfo->cpumaplen)); /* Append to iothreadinfolist */ iothreadinfolist[niothreads] = g_steal_pointer(&iothreadinfo); diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index e2a08d737e..35966a5a8d 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -652,9 +652,7 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef, cpumask = bitmap; } } - if (virBitmapToData(cpumask, &info_ret[i]->cpumap, - &info_ret[i]->cpumaplen) < 0) - goto cleanup; + virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen); } *info = g_steal_pointer(&info_ret); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6ce949dd07..59ba6cfbf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4787,8 +4787,7 @@ qemuDomainGetIOThreadsLive(virDomainObj *vm, if (!(map = virProcessGetAffinity(iothreads[i]->thread_id))) goto endjob; - if (virBitmapToData(map, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) - goto endjob; + virBitmapToData(map, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen); } *info = g_steal_pointer(&info_ret); diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index e42f5b872c..8a3f33c806 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -637,10 +637,8 @@ virBitmapNewData(const void *data, * Convert a bitmap to a chunk of data containing bits information. * Data consists of sequential bytes, with lower bytes containing * lower bits. This function allocates @data. - * - * Returns 0 on success, -1 otherwise. */ -int +void virBitmapToData(virBitmap *bitmap, unsigned char **data, int *dataLen) @@ -656,8 +654,6 @@ virBitmapToData(virBitmap *bitmap, *dataLen = len; virBitmapToDataBuf(bitmap, *data, *dataLen); - - return 0; } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index a9f9d97fd0..a9c0a1a307 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -91,7 +91,7 @@ virBitmap *virBitmapNewCopy(virBitmap *src) ATTRIBUTE_NONNULL(1); virBitmap *virBitmapNewData(const void *data, int len) ATTRIBUTE_NONNULL(1); -int virBitmapToData(virBitmap *bitmap, unsigned char **data, int *dataLen) +void virBitmapToData(virBitmap *bitmap, unsigned char **data, int *dataLen) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void virBitmapToDataBuf(virBitmap *bitmap, unsigned char *data, size_t len) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 3b4a11effb..80819d83e6 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1110,8 +1110,8 @@ virHostCPUGetMap(unsigned char **cpumap, if (!(cpus = virHostCPUGetOnlineBitmap())) goto cleanup; - if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0) - goto cleanup; + if (cpumap) + virBitmapToData(cpus, cpumap, &dummy); if (online) *online = virBitmapCountBits(cpus); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 27b6c13114..709c62ab54 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -339,8 +339,7 @@ test5(const void *v G_GNUC_UNUSED) ignore_value(virBitmapSetBit(bitmap, 2)); ignore_value(virBitmapSetBit(bitmap, 15)); - if (virBitmapToData(bitmap, &data2, &len2) < 0) - return -1; + virBitmapToData(bitmap, &data2, &len2); if (len2 != sizeof(data) || data2[0] != 0x05 || diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 0a3a113dca..b020d87f49 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -794,8 +794,7 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl, if (!(onlineVcpus = virBitmapParseUnlimited(onlineVcpuStr))) goto cleanup; - if (virBitmapToData(onlineVcpus, &onlineVcpumap, &dummy) < 0) - goto cleanup; + virBitmapToData(onlineVcpus, &onlineVcpumap, &dummy); if (enable) { offlinableVcpuStr = vshGetTypedParamValue(ctl, &params[2]); @@ -803,8 +802,7 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl, if (!(offlinableVcpus = virBitmapParseUnlimited(offlinableVcpuStr))) goto cleanup; - if (virBitmapToData(offlinableVcpus, &offlinableVcpumap, &dummy) < 0) - goto cleanup; + virBitmapToData(offlinableVcpus, &offlinableVcpumap, &dummy); lastcpu = virBitmapLastSetBit(offlinableVcpus); cpulist = g_new0(char *, nvcpus - virBitmapCountBits(onlineVcpus) + 1); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ceff6789d8..a2e30a0fa9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7420,8 +7420,7 @@ virshParseCPUList(vshControl *ctl, int *cpumaplen, } } - if (virBitmapToData(map, &cpumap, cpumaplen) < 0) - return NULL; + virBitmapToData(map, &cpumap, cpumaplen); return cpumap; } -- 2.42.4

2 2

[PATCH] nss: Fix memory leak in findLease
by Alexander Kuznetsov 15 Apr '25

15 Apr '25

path is allocated by asprintf() and must be freed later if realloc() fails or at the end of each while() iteration Move the free() call out of LIBVIRT_NSS_GUEST macro and add another one if realloc() fails Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Dmitry Fedin <d.fedin(a)fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam(a)altlinux.org> --- tools/nss/libvirt_nss.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index d79a00a1b0..190cc7a3dd 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -141,8 +141,11 @@ findLease(const char *name, goto cleanup; tmpLease = realloc(leaseFiles, sizeof(char *) * (nleaseFiles + 1)); - if (!tmpLease) + if (!tmpLease) { + free(path); goto cleanup; + } + leaseFiles = tmpLease; leaseFiles[nleaseFiles++] = path; #if defined(LIBVIRT_NSS_GUEST) @@ -155,8 +158,8 @@ findLease(const char *name, free(path); goto cleanup; } - free(path); #endif /* LIBVIRT_NSS_GUEST */ + free(path); } errno = 0; -- 2.42.4

4 9

[PATCH] virNWFilterIncludeDefToRuleInst: Prevent potential double g_free
by Alexander Kuznetsov 15 Apr '25

15 Apr '25

If virNWFilterDefToInst returns -1, it has already called virNWFilterInstReset. Remove the additional call to prevent a double g_free Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Dmitry Fedin <d.fedin(a)fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam(a)altlinux.org> --- src/nwfilter/nwfilter_gentech_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 41f270bb7c..f7a909bdc0 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -293,10 +293,8 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverState *driver, tmpvars, useNewFilter, foundNewFilter, - inst) < 0) { - virNWFilterInstReset(inst); + inst) < 0) return -1; - } return 0; } -- 2.42.4

3 4

[PATCH] virsh: prevent potential NULL dereference
by Alexander Kuznetsov 15 Apr '25

15 Apr '25

virXPathString() can return NULL so we need to check it before calling strcmp() Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Dmitry Fedin <d.fedin(a)fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam(a)altlinux.org> --- tools/virsh-completer-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 0a3a113dca..abacfd04c1 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -891,7 +891,7 @@ virshDomainConsoleCompleter(vshControl *ctl, ctxt->node = parallels[i - nserials]; type = virXPathString("string(./@type)", ctxt); - if (STRNEQ(type, "pty")) + if (!type || STRNEQ(type, "pty")) continue; tmp[offset++] = virXPathString("string(./alias/@name)", ctxt); -- 2.42.4

3 4

[PATCH v2 0/2] conf: use const virDomainDef pointers
by Roman Bogorodskiy 15 Apr '25

15 Apr '25

Changes since v1: - Split domain_conf and bhyve changes into separate commits - Convert virDomainNetNotifyActualDevice() as well Roman Bogorodskiy (2): conf: use const virDomainDef pointers bhyve: use const virDomainDef pointer in bhyveBuildNetArgStr() src/bhyve/bhyve_command.c | 2 +- src/conf/domain_conf.c | 10 +++++----- src/conf/domain_conf.h | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) -- 2.49.0

2 3

[PATCH] bhyve: capabilities: advertise RNG device support
by Roman Bogorodskiy 14 Apr '25

14 Apr '25

Signed-off-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com> --- src/bhyve/bhyve_capabilities.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index 37ae5d2872..36a092a819 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -109,6 +109,16 @@ virBhyveDomainCapsFill(virDomainCaps *caps, VIR_DOMAIN_CAPS_ENUM_SET(caps->video.modelType, VIR_DOMAIN_VIDEO_TYPE_GOP); } + if (bhyvecaps & BHYVE_CAP_VIRTIO_RND) { + caps->rng.supported = VIR_TRISTATE_BOOL_YES; + caps->rng.model.report = true; + caps->rng.backendModel.report = true; + + VIR_DOMAIN_CAPS_ENUM_SET(caps->rng.model, VIR_DOMAIN_RNG_MODEL_VIRTIO); + VIR_DOMAIN_CAPS_ENUM_SET(caps->rng.backendModel, + VIR_DOMAIN_RNG_BACKEND_RANDOM); + } + caps->hostdev.supported = VIR_TRISTATE_BOOL_NO; caps->features[VIR_DOMAIN_CAPS_FEATURE_IOTHREADS] = VIR_TRISTATE_BOOL_NO; caps->features[VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO] = VIR_TRISTATE_BOOL_NO; -- 2.49.0

2 1

[PATCH v1 0/1] rpc: Re-read the data if EAGAIN or EINTR were captured
by yong.huang＠smartx.com 14 Apr '25

14 Apr '25

From: Hyman Huang <yong.huang(a)smartx.com> v1: 1. Encapsulate the retry logic inside virNetTLSSession{Read,Write} 2. Use VIR_DEBUG instead of VIR_WARN to log the retry operation rfc: https://patchew.org/Libvirt/d716a59dc2c61916917c6d2e07d62055745606d5.174404… Please review, thanks. Yong Hyman Huang (1): rpc: Add the retry argument for virNetTLSSession{Read,Write} src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- src/rpc/virnettlscontext.c | 28 ++++++++++++++++++++++++++-- src/rpc/virnettlscontext.h | 6 ++++-- 4 files changed, 33 insertions(+), 7 deletions(-) -- 2.27.0

3 4