[libvirt] [PATCH 0/2] Address some recent coverity
The first patch is a noise cleaner, while the second one fixes a real leak. Also anyone with desire to 'debug' a build environment - seems when the STATIC_ANALYSIS is true, the building of the docs (apibuild.py[.stamp]) fails in a rather spectacular mess. I can avoid building docs, but it's not clear to me what the failure is... John Ferlan (2): qemu: More qemu_monitor_json cleanups libxl: Resolve Coverity RESOURCE_LEAK src/libxl/libxl_migration.c | 5 ++++- src/qemu/qemu_monitor_json.c | 46 ++++++++++++++++++++++++-------------------- 2 files changed, 29 insertions(+), 22 deletions(-) -- 2.5.5
Recent adjustments to the code produced a litany of coverity false positives, but only because the "standard" procedure of setting a variable to NULL after it was assigned to something else and keeping the *Free/*FREE call in the cleanup path wasn't kept. So this patch makes those adjustments (assign variable to NULL and remove the if 'ret < 0' condition to clean it up). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 46 ++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4cf8d75..1790e4d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4863,9 +4863,10 @@ int qemuMonitorJSONGetMachines(qemuMonitorPtr mon, ret = n; *machines = infolist; + infolist = NULL; cleanup: - if (ret < 0 && infolist) { + if (infolist) { for (i = 0; i < n; i++) qemuMonitorMachineInfoFree(infolist[i]); VIR_FREE(infolist); @@ -4939,10 +4940,10 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, ret = n; *cpus = cpulist; + cpulist = NULL; cleanup: - if (ret < 0) - virStringFreeList(cpulist); + virStringFreeList(cpulist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5003,10 +5004,11 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, ret = n; *commands = commandlist; + commandlist = NULL; + cleanup: - if (ret < 0) - virStringFreeList(commandlist); + virStringFreeList(commandlist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5072,10 +5074,10 @@ int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, ret = n; *events = eventlist; + eventlist = NULL; cleanup: - if (ret < 0) - virStringFreeList(eventlist); + virStringFreeList(eventlist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5189,6 +5191,7 @@ qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, ret = n; *params = paramlist; + paramlist = NULL; cleanup: /* If we failed before getting the JSON array of options, we (try) @@ -5196,8 +5199,7 @@ qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, if (!qemuMonitorGetOptions(mon)) qemuMonitorSetOptions(mon, virJSONValueNewArray()); - if (ret < 0) - virStringFreeList(paramlist); + virStringFreeList(paramlist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5306,10 +5308,10 @@ int qemuMonitorJSONGetObjectTypes(qemuMonitorPtr mon, ret = n; *types = typelist; + typelist = NULL; cleanup: - if (ret < 0) - virStringFreeList(typelist); + virStringFreeList(typelist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5389,9 +5391,10 @@ int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, ret = n; *paths = pathlist; + pathlist = NULL; cleanup: - if (ret < 0 && pathlist) { + if (pathlist) { for (i = 0; i < n; i++) qemuMonitorJSONListPathFree(pathlist[i]); VIR_FREE(pathlist); @@ -5616,10 +5619,10 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, ret = n; *props = proplist; + proplist = NULL; cleanup: - if (ret < 0) - virStringFreeList(proplist); + virStringFreeList(proplist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5726,10 +5729,10 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon, ret = n; *capabilities = list; + list = NULL; cleanup: - if (ret < 0) - virStringFreeList(list); + virStringFreeList(list); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -5911,10 +5914,10 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, ret = n; *capabilities = list; + list = NULL; cleanup: - if (ret < 0) - VIR_FREE(list); + VIR_FREE(list); virJSONValueFree(cmd); virJSONValueFree(reply); @@ -6125,10 +6128,10 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon, const char *qmpCmd, ret = n; *array = list; + list = NULL; cleanup: - if (ret < 0) - virStringFreeList(list); + virStringFreeList(list); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -6668,9 +6671,10 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, ret = n; *iothreads = infolist; + infolist = NULL; cleanup: - if (ret < 0 && infolist) { + if (infolist) { for (i = 0; i < n; i++) VIR_FREE(infolist[i]); VIR_FREE(infolist); -- 2.5.5
On Mon, May 16, 2016 at 10:05:04AM -0400, John Ferlan wrote:
Recent adjustments to the code produced a litany of coverity false positives, but only because the "standard" procedure of setting a variable to NULL after it was assigned to something else and keeping the *Free/*FREE call in the cleanup path wasn't kept. So this patch makes those adjustments (assign variable to NULL and remove the if 'ret < 0' condition to clean it up).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 46 ++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 21 deletions(-)
ACK Jan
Commit id 'f9edcfa4' added cookie manipulation for libxl; however, some cookie crumb cleanup was missed. In libxlDomainMigrationBegin, the cookie is allocated and baked; however, the mig ingredients weren't cleaned up. In libxlDomainMigrationPrepare, when the 'mig' cookie is added to the args, set the 'mig = NULL'; otherwise, other failure paths between when the code ate the cookie data and when it was added to args would fail to clean up the crumbs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index ad54960..a809aa0 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -402,7 +402,7 @@ libxlDomainMigrationBegin(virConnectPtr conn, { libxlDriverPrivatePtr driver = conn->privateData; libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - libxlMigrationCookiePtr mig; + libxlMigrationCookiePtr mig = NULL; virDomainDefPtr tmpdef = NULL; virDomainDefPtr def; char *xml = NULL; @@ -440,6 +440,7 @@ libxlDomainMigrationBegin(virConnectPtr conn, vm = NULL; cleanup: + libxlMigrationCookieFree(mig); if (vm) virObjectUnlock(vm); @@ -601,6 +602,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, args->socks = socks; args->nsocks = nsocks; args->migcookie = mig; + mig = NULL; for (i = 0; i < nsocks; i++) { if (virNetSocketSetBlocking(socks[i], true) < 0) @@ -640,6 +642,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, } done: + libxlMigrationCookieFree(mig); if (!uri_in) VIR_FREE(hostname); else -- 2.5.5
The commit summary should describe the change being made, e.g: s/Resolve Coverity RESOURCE_LEAK/free migration cookie/ On Mon, May 16, 2016 at 10:05:05AM -0400, John Ferlan wrote:
Commit id 'f9edcfa4' added cookie manipulation for libxl; however, some cookie crumb cleanup was missed.
In libxlDomainMigrationBegin, the cookie is allocated and baked; however, the mig ingredients weren't cleaned up.
In libxlDomainMigrationPrepare, when the 'mig' cookie is added to the args, set the 'mig = NULL'; otherwise, other failure paths between when the code ate the cookie data and when it was added to args would fail to clean up the crumbs.
Crediting Coverity would better fit here in the commit message body, e.g.: Found by Cinderel^WCoverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
ACK Jan
participants (2)
-
John Ferlan -
Ján Tomko