[libvirt] [PATCH 0/6] Some Coverity patches
Resolve a few things that have built up previous and a couple that are more recent. John Ferlan (6): vbox: Fix resource leak vbox: Fix resource leak qemu: Fix Coverity build for qemu_monitor test: Fix resource leak in qemumonitorjsontest test: Check return status for libxlxml2domconfigtest conf: Check error from virXMLFormatElement call src/conf/domain_conf.c | 3 ++- src/qemu/qemu_monitor.h | 2 +- src/vbox/vbox_common.c | 28 +++++++++++++++------------- tests/libxlxml2domconfigtest.c | 4 +++- tests/qemumonitorjsontest.c | 2 +- 5 files changed, 22 insertions(+), 17 deletions(-) -- 2.14.4
Need to free the allocated hardDiskToOpen array. The contents of the array are just pointers returned by virVBoxSnapshotConfHardDiskByLocation and not allocated AFAICT so they don't need to also be freed as well. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 72a24a3464..0e7fe06748 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4627,6 +4627,8 @@ vboxSnapshotRedefine(virDomainPtr dom, int realReadOnlyDisksPathSize = 0; virVBoxSnapshotConfSnapshotPtr newSnapshotPtr = NULL; unsigned char snapshotUuid[VIR_UUID_BUFLEN]; + virVBoxSnapshotConfHardDiskPtr *hardDiskToOpen = NULL; + size_t hardDiskToOpenSize = 0; char **searchResultTab = NULL; ssize_t resultSize = 0; int it = 0; @@ -5080,8 +5082,6 @@ vboxSnapshotRedefine(virDomainPtr dom, */ for (it = 0; it < def->dom->ndisks; it++) { char *location = NULL; - virVBoxSnapshotConfHardDiskPtr *hardDiskToOpen = NULL; - size_t hardDiskToOpenSize = 0; location = def->dom->disks[it]->src->path; if (!location) @@ -5394,8 +5394,7 @@ vboxSnapshotRedefine(virDomainPtr dom, if (!location) goto cleanup; - virVBoxSnapshotConfHardDiskPtr *hardDiskToOpen = NULL; - size_t hardDiskToOpenSize = virVBoxSnapshotConfDiskListToOpen(snapshotMachineDesc, + hardDiskToOpenSize = virVBoxSnapshotConfDiskListToOpen(snapshotMachineDesc, &hardDiskToOpen, location); for (jt = 0; jt < hardDiskToOpenSize; jt++) { IMedium *medium = NULL; @@ -5459,6 +5458,7 @@ vboxSnapshotRedefine(virDomainPtr dom, virStringListFree(realReadOnlyDisksPath); virStringListFree(realReadWriteDisksPath); virStringListFree(searchResultTab); + VIR_FREE(hardDiskToOpen); VIR_FREE(newSnapshotPtr); VIR_FREE(machineLocationPath); VIR_FREE(nameTmpUse); -- 2.14.4
The @disk was allocated, filled in, and consumed on the normal path, but for error/cleanup paths it would be leaked. Rename to newHardDisk and manage properly. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 0e7fe06748..664650f217 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4629,6 +4629,7 @@ vboxSnapshotRedefine(virDomainPtr dom, unsigned char snapshotUuid[VIR_UUID_BUFLEN]; virVBoxSnapshotConfHardDiskPtr *hardDiskToOpen = NULL; size_t hardDiskToOpenSize = 0; + virVBoxSnapshotConfHardDiskPtr newHardDisk = NULL; char **searchResultTab = NULL; ssize_t resultSize = 0; int it = 0; @@ -5236,7 +5237,6 @@ vboxSnapshotRedefine(virDomainPtr dom, PRUnichar *newLocation = NULL; char *newLocationUtf8 = NULL; resultCodeUnion resultCode; - virVBoxSnapshotConfHardDiskPtr disk = NULL; char *uuid = NULL; char *format = NULL; char *tmp = NULL; @@ -5301,11 +5301,11 @@ vboxSnapshotRedefine(virDomainPtr dom, } VBOX_RELEASE(progress); /* - * The differential disk is created, we add it to the media registry and the - * machine storage controllers. + * The differential newHardDisk is created, we add it to the + * media registry and the machine storage controllers. */ - if (VIR_ALLOC(disk) < 0) + if (VIR_ALLOC(newHardDisk) < 0) goto cleanup; rc = gVBoxAPI.UIMedium.GetId(newMedium, &iid); @@ -5316,24 +5316,25 @@ vboxSnapshotRedefine(virDomainPtr dom, goto cleanup; } gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); - disk->uuid = uuid; + newHardDisk->uuid = uuid; vboxIIDUnalloc(&iid); - if (VIR_STRDUP(disk->location, newLocationUtf8) < 0) + if (VIR_STRDUP(newHardDisk->location, newLocationUtf8) < 0) goto cleanup; rc = gVBoxAPI.UIMedium.GetFormat(newMedium, &formatUtf16); VBOX_UTF16_TO_UTF8(formatUtf16, &format); - disk->format = format; + newHardDisk->format = format; VBOX_UTF16_FREE(formatUtf16); - if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(disk, + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(newHardDisk, snapshotMachineDesc->mediaRegistry, parentUuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to add hard disk to the media registry")); goto cleanup; } + newHardDisk = NULL; /* Consumed by above */ /*Adding the fake disk to the machine storage controllers*/ resultSize = virStringSearch(snapshotMachineDesc->storageController, @@ -5348,7 +5349,7 @@ vboxSnapshotRedefine(virDomainPtr dom, tmp = virStringReplace(snapshotMachineDesc->storageController, searchResultTab[it], - disk->uuid); + uuid); VIR_FREE(snapshotMachineDesc->storageController); if (!tmp) goto cleanup; @@ -5458,6 +5459,7 @@ vboxSnapshotRedefine(virDomainPtr dom, virStringListFree(realReadOnlyDisksPath); virStringListFree(realReadWriteDisksPath); virStringListFree(searchResultTab); + virVboxSnapshotConfHardDiskFree(newHardDisk); VIR_FREE(hardDiskToOpen); VIR_FREE(newSnapshotPtr); VIR_FREE(machineLocationPath); -- 2.14.4
Commit id '7ef0471bf' added a new parameter to qemuMonitorOpen, but didn't update the ATTTRIBUTE_NONNULL for the @cb (param 5). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 43843721bf..d01266ff86 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -317,7 +317,7 @@ qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, unsigned long long timeout, qemuMonitorCallbacksPtr cb, void *opaque) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6); qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm, int sockfd, bool json, -- 2.14.4
Introduced by commmit id 37bd4571c. Need to goto cleanup and not return directly. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemumonitorjsontest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 3b494a1dba..2eefd06b6e 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2693,7 +2693,7 @@ testQemuMonitorCPUInfo(const void *opaque) vm = qemuMonitorTestGetDomainObj(test); if (!vm) - return -1; + goto cleanup; rc = qemuMonitorGetCPUInfo(qemuMonitorTestGetMonitor(test), &vcpus, data->maxvcpus, true, data->fast); -- 2.14.4
Commit id d8e8b63d introduced the test, but neglected to check for error from virTestLoadFile in testCompareXMLToDomConfig. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/libxlxml2domconfigtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index 0d2a7385e5..54a92cc959 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -104,7 +104,9 @@ testCompareXMLToDomConfig(const char *xmlfile, goto cleanup; } - virTestLoadFile(jsonfile, &tempjson); + if (virTestLoadFile(jsonfile, &tempjson) < 0) + goto cleanup; + if (libxl_domain_config_from_json(cfg->ctx, &expectconfig, tempjson) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Failed to create libxl_domain_config from JSON doc"); -- 2.14.4
Commit id 1bd5a08d added a call to virXMLFormatElement without also checking the return status. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ab93bb7b45..1157c7dd93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27522,7 +27522,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, unit, short_size); } - virXMLFormatElement(buf, "smm", &attrBuf, &childBuf); + if (virXMLFormatElement(buf, "smm", &attrBuf, &childBuf) < 0) + goto error; } break; -- 2.14.4
On Fri, Jun 08, 2018 at 01:10:13PM -0400, John Ferlan wrote:
Resolve a few things that have built up previous and a couple that are more recent.
John Ferlan (6): vbox: Fix resource leak vbox: Fix resource leak qemu: Fix Coverity build for qemu_monitor test: Fix resource leak in qemumonitorjsontest test: Check return status for libxlxml2domconfigtest conf: Check error from virXMLFormatElement call
src/conf/domain_conf.c | 3 ++- src/qemu/qemu_monitor.h | 2 +- src/vbox/vbox_common.c | 28 +++++++++++++++------------- tests/libxlxml2domconfigtest.c | 4 +++- tests/qemumonitorjsontest.c | 2 +- 5 files changed, 22 insertions(+), 17 deletions(-)
Reviewed-by: Katerina Koukiou <kkoukiou@redhat.com>
participants (2)
-
John Ferlan -
Katerina Koukiou