[libvirt] [PATCH 0/8] Resolve new Coverity issues
Installed a new Coverity checker (7.6.1) and naturally new issues were found, plus a set of false positives which for now I'll just have to ignore since it seems they may be a bug in Coverity, but that's still to be determined John Ferlan (8): libxl: Resolve Coverity RESOURCE_LEAK vbox: Resolve Coverity RESOURCE_LEAK qemu: Resolve Coverity IDENTICAL_BRANCHES qemu: Resolve Coverity FORWARD_NULL xen: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL src/libxl/libxl_migration.c | 1 + src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_command.c | 4 +++- src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_hotplug.c | 11 +++-------- src/vbox/vbox_common.c | 21 +++++++-------------- src/xen/xm_internal.c | 3 +++ 7 files changed, 23 insertions(+), 26 deletions(-) -- 2.1.0
The returned socks from virNetSocketNewListenTCP needs to be VIR_FREE'd as well as seach of the Close/Unref on all the socks[i] that is already done Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 1efd98f..10b5bd6 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -445,6 +445,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virNetSocketClose(socks[i]); virObjectUnref(socks[i]); } + VIR_FREE(socks); /* Remove virDomainObj from domain list */ if (vm) { virDomainObjListRemove(driver->domains, vm); -- 2.1.0
If the virStringSearch() returns a 0 (zero), then each of the uses of the call will just jump to cleanup forgetting to free the returned empty list. Expand the scope a bit of each use and free at cleanup. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 0bb5d29..e3a1739 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4463,6 +4463,8 @@ vboxSnapshotRedefine(virDomainPtr dom, int realReadOnlyDisksPathSize = 0; virVBoxSnapshotConfSnapshotPtr newSnapshotPtr = NULL; unsigned char snapshotUuid[VIR_UUID_BUFLEN]; + char **searchResultTab = NULL; + ssize_t resultSize = 0; int it = 0; int jt = 0; PRUint32 aMediaSize = 0; @@ -4862,8 +4864,6 @@ vboxSnapshotRedefine(virDomainPtr dom, for (it = 0; it < def->dom->ndisks; it++) { char *location = NULL; const char *uuidReplacing = NULL; - char **searchResultTab = NULL; - ssize_t resultSize = 0; char *tmp = NULL; location = def->dom->disks[it]->src->path; @@ -4885,6 +4885,7 @@ vboxSnapshotRedefine(virDomainPtr dom, searchResultTab[it], uuidReplacing); virStringFreeList(searchResultTab); + searchResultTab = NULL; VIR_FREE(newSnapshotPtr->storageController); if (!tmp) goto cleanup; @@ -5029,8 +5030,6 @@ vboxSnapshotRedefine(virDomainPtr dom, if (needToChangeStorageController) { /*We need to append this disk in the storage controller*/ - char **searchResultTab = NULL; - ssize_t resultSize = 0; char *tmp = NULL; resultSize = virStringSearch(snapshotMachineDesc->storageController, VBOX_UUID_REGEX, @@ -5045,7 +5044,6 @@ vboxSnapshotRedefine(virDomainPtr dom, tmp = virStringReplace(snapshotMachineDesc->storageController, searchResultTab[it], disk->uuid); - virStringFreeList(searchResultTab); VIR_FREE(snapshotMachineDesc->storageController); if (!tmp) goto cleanup; @@ -5077,8 +5075,6 @@ vboxSnapshotRedefine(virDomainPtr dom, virVBoxSnapshotConfHardDiskPtr disk = NULL; char *uuid = NULL; char *format = NULL; - char **searchResultTab = NULL; - ssize_t resultSize = 0; char *tmp = NULL; vboxIIDUnion iid, parentiid; @@ -5189,7 +5185,6 @@ vboxSnapshotRedefine(virDomainPtr dom, tmp = virStringReplace(snapshotMachineDesc->storageController, searchResultTab[it], disk->uuid); - virStringFreeList(searchResultTab); VIR_FREE(snapshotMachineDesc->storageController); if (!tmp) goto cleanup; @@ -5299,6 +5294,7 @@ vboxSnapshotRedefine(virDomainPtr dom, VBOX_UTF8_FREE(machineName); virStringFreeList(realReadOnlyDisksPath); virStringFreeList(realReadWriteDisksPath); + virStringFreeList(searchResultTab); VIR_FREE(newSnapshotPtr); VIR_FREE(machineLocationPath); VIR_FREE(nameTmpUse); @@ -6730,6 +6726,8 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) char *settingsFilepath = NULL; virVBoxSnapshotConfMachinePtr snapshotMachineDesc = NULL; int isCurrent = -1; + char **searchResultTab = NULL; + ssize_t resultSize = 0; int it = 0; PRUnichar *machineNameUtf16 = NULL; char *machineName = NULL; @@ -6822,8 +6820,6 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) virVBoxSnapshotConfHardDiskPtr disk = NULL; char *uuid = NULL; char *format = NULL; - char **searchResultTab = NULL; - ssize_t resultSize = 0; char *tmp = NULL; vboxIIDUnion iid, parentiid; resultCodeUnion resultCode; @@ -6950,7 +6946,6 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) tmp = virStringReplace(snapshotMachineDesc->storageController, searchResultTab[it], disk->uuid); - virStringFreeList(searchResultTab); VIR_FREE(snapshotMachineDesc->storageController); if (!tmp) goto cleanup; @@ -6970,8 +6965,6 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) } else { for (it = 0; it < def->dom->ndisks; it++) { const char *uuidRO = NULL; - char **searchResultTab = NULL; - ssize_t resultSize = 0; char *tmp = NULL; uuidRO = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc, def->dom->disks[it]->src->path); @@ -6996,7 +6989,6 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) tmp = virStringReplace(snapshotMachineDesc->storageController, searchResultTab[it], uuidRO); - virStringFreeList(searchResultTab); VIR_FREE(snapshotMachineDesc->storageController); if (!tmp) goto cleanup; @@ -7147,6 +7139,7 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) VBOX_RELEASE(machine); VBOX_UTF16_FREE(settingsFilePathUtf16); VBOX_UTF8_FREE(settingsFilepath); + virStringFreeList(searchResultTab); VIR_FREE(snapshotMachineDesc); VBOX_UTF16_FREE(machineNameUtf16); VBOX_UTF8_FREE(machineName); -- 2.1.0
Coverity complains that in the error paths both the < 0 condition and the success path after the qemuDomainObjExitMonitor failure will end up going to cleanup. So just use ignore_value in this error path to resolve the complaint. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 613b728..4e9603e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3640,15 +3640,13 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } } @@ -4136,8 +4134,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.1.0
Coverity notes that ->ifname is used after the VIR_FREE done in the code path after the call to virNetDevMacVLanDeleteWithVPortProfile by a call to virNetDevOpenvswitchRemovePort. Since the ->ifname will be VIR_FREE()'d eventually in virDomainNetDefFree just remove the extraneous VIR_FREE here. When originally added, the Openvswitch code wasn't present and checks were made for non NULL prior to use. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4e9603e..095ed88 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1147,7 +1147,6 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, virDomainNetGetActualDirectMode(net), virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); - VIR_FREE(net->ifname); } vport = virDomainNetGetActualVirtPortProfile(net); @@ -3107,7 +3106,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainNetGetActualDirectMode(net), virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); - VIR_FREE(net->ifname); } vport = virDomainNetGetActualVirtPortProfile(net); -- 2.1.0
Coverity found that xenXMConfigCacheAddFile has an error path in which no error message and a -1 was not returned which could have resulted in a NULL dereference in a VIR_DEBUG statement and of course an erroneous 0 value returned! Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xen/xm_internal.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 64752df..59b1cd4 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -279,6 +279,9 @@ xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) virDomainDefFree(entry->def); VIR_FREE(entry->filename); VIR_FREE(entry); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("xenXMConfigCacheRefresh: virHashAddEntry name")); + return -1; } } VIR_DEBUG("Added config %s %s", entry->def->name, filename); -- 2.1.0
Coverity points out it was possible to have a zero return from qemuBuildRNGBackendProps thus not filling in 'props' and then causing a NULL dereference on the next call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c32d8c6..68844fa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6464,7 +6464,9 @@ qemuBuildRNGBackendProps(virDomainRNGDefPtr rng, break; case VIR_DOMAIN_RNG_BACKEND_LAST: - break; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unknown rng-random backend")); + goto cleanup; } ret = 0; -- 2.1.0
Coverity complains over the [n]values pairing in virQEMUCapsFreeStringList and rather than make a bunch if "if values" checks prior to calling, by just adding the values check inside the free function we avoid the chance that somehow nvalues is > 0, while values == NULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d50863f..25c15bf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1702,6 +1702,8 @@ virQEMUCapsFreeStringList(size_t len, char **values) { size_t i; + if (!values) + return; for (i = 0; i < len; i++) VIR_FREE(values[i]); VIR_FREE(values); @@ -1794,7 +1796,7 @@ virQEMUCapsParseDeviceStrObjectProps(const char *str, ret = nproplist; cleanup: - if (ret < 0 && proplist) + if (ret < 0) virQEMUCapsFreeStringList(nproplist, proplist); return ret; } -- 2.1.0
Coverity points out that qemuMonitorGetAllBlockStatsInfo could return a -1 and thus not fill in 'stats' (leaving it NULL). Then the call to qemuMonitorBlockStatsUpdateCapacity will dereference it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a76858a..d173aa1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19633,8 +19633,9 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, dom); rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats, visitBacking); - ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, - visitBacking)); + if (rc >= 0) + ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, + visitBacking)); if (qemuDomainObjExitMonitor(driver, dom) < 0) goto cleanup; -- 2.1.0
On 05.05.2015 13:34, John Ferlan wrote:
Installed a new Coverity checker (7.6.1) and naturally new issues were found, plus a set of false positives which for now I'll just have to ignore since it seems they may be a bug in Coverity, but that's still to be determined
John Ferlan (8): libxl: Resolve Coverity RESOURCE_LEAK vbox: Resolve Coverity RESOURCE_LEAK qemu: Resolve Coverity IDENTICAL_BRANCHES qemu: Resolve Coverity FORWARD_NULL xen: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL
src/libxl/libxl_migration.c | 1 + src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_command.c | 4 +++- src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_hotplug.c | 11 +++-------- src/vbox/vbox_common.c | 21 +++++++-------------- src/xen/xm_internal.c | 3 +++ 7 files changed, 23 insertions(+), 26 deletions(-)
ACK series. Michal
participants (2)
-
John Ferlan -
Michal Privoznik