[libvirt PATCH 0/3] Fix some memory leaks
These memory leaks were found by llvm's address sanitizer asan. Tim Wiederhake (3): xenParseHypervisorFeatures: Fix memory leak virDomainFeaturesDefParse: Fix memory leak virQEMUCapsSetHostModel: Fix memory leak src/conf/domain_conf.c | 2 +- src/libxl/xen_common.c | 1 + src/qemu/qemu_capabilities.c | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/xen_common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 12a44280cb..ad5a3de116 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -567,6 +567,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT; def->clock.timers[def->clock.ntimers - 1] = timer; + VIR_FREE(strval); } if (xenConfigGetString(conf, "passthrough", &strval, NULL) < 0) -- 2.26.2
On Thu, Apr 15, 2021 at 17:08:26 +0200, Tim Wiederhake wrote: Fixes: b523e22521a
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/xen_common.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 12a44280cb..ad5a3de116 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -567,6 +567,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
def->clock.timers[def->clock.ntimers - 1] = timer; + VIR_FREE(strval);
While this fixes the specific problem, it still leaves possibility for making a mistake again since 'strval' isn't explicitly freed after the second use. A preferrable fix is to use one variable per use when used with g_autofree or remove g_autofree and explicitly free it after both uses. CCing Jim to state his preferred solution.
}
if (xenConfigGetString(conf, "passthrough", &strval, NULL) < 0) -- 2.26.2
On 4/16/21 2:25 AM, Peter Krempa wrote:
On Thu, Apr 15, 2021 at 17:08:26 +0200, Tim Wiederhake wrote:
Fixes: b523e22521a
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/xen_common.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 12a44280cb..ad5a3de116 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -567,6 +567,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
def->clock.timers[def->clock.ntimers - 1] = timer; + VIR_FREE(strval);
While this fixes the specific problem, it still leaves possibility for making a mistake again since 'strval' isn't explicitly freed after the second use.
A preferrable fix is to use one variable per use when used with g_autofree or remove g_autofree and explicitly free it after both uses.
Since there are only two uses, I prefer this approach as well. E.g. g_autofree char *tscmode = NULL; g_autofree char *passthrough = NULL; Regards, Jim
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 029f2d8d9c..5591681283 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18994,12 +18994,12 @@ virDomainFeaturesDefParse(virDomainDef *def, int feature; int value; g_autofree char *ptval = NULL; - g_autofree char *tmp = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; feature = virDomainXenTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.26.2
On Thu, Apr 15, 2021 at 17:08:27 +0200, Tim Wiederhake wrote: Fixes: 94013ee04e3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
virQEMUCapsSetHostModel is called by virQEMUCapsInitHostCPUModel, which in turn is typically called twice (for KVM and QEMU), e.g. in virQEMUCapsLoadCache and virQEMUCapsNewForBinaryInternal. The second call leaks memory pointed to by "reported", "migratable" and "full". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f1a3c526ef..ff8877631c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2276,6 +2276,14 @@ virQEMUCapsSetHostModel(virQEMUCaps *qemuCaps, virQEMUCapsHostCPUData *cpuData; cpuData = &virQEMUCapsGetAccel(qemuCaps, type)->hostCPU; + + if (cpuData->reported) + virCPUDefFree(cpuData->reported); + if (cpuData->migratable) + virCPUDefFree(cpuData->migratable); + if (cpuData->full) + virCPUDefFree(cpuData->full); + cpuData->reported = reported; cpuData->migratable = migratable; cpuData->full = full; -- 2.26.2
On Thu, Apr 15, 2021 at 17:08:28 +0200, Tim Wiederhake wrote:
virQEMUCapsSetHostModel is called by virQEMUCapsInitHostCPUModel, which in turn is typically called twice (for KVM and QEMU), e.g. in virQEMUCapsLoadCache and virQEMUCapsNewForBinaryInternal.
The second call leaks memory pointed to by "reported", "migratable" and "full".
That sounds to me like a usage problem in the caller, where if it's called twice it's either using a non-cleared variable, or the value will be overwritten and thus calling it in the first place is probably dubious, so the proposed patch feels to me like it's fixing a symptom rather than the real problem.
participants (3)
-
Jim Fehlig -
Peter Krempa -
Tim Wiederhake