[libvirt] [PATCH 0/6] More Coverity changes
Like a gift that keeps on giving... More Coverity changes. Adjust changes from earlier based on Eric's comments/feedback. nodeinfo.c - use sa_assert() instead of comment: https://www.redhat.com/archives/libvir-list/2013-January/msg01578.html virnetclient.c - remove change - the VIR_FREE change resolves this: https://www.redhat.com/archives/libvir-list/2013-January/msg01581.html Two new Coverity warnings popped up today (lxc_driver.c & lxc_process.c). No rhyme or reason, they just were new with no recent change to blame (very strange). I missed the virnettlscontexttest.c and it's VIR_FREE() warning this morning because it's before all the BAD_SIZEOF. The virbitmaptest.c change was on another branch waiting - I just moved it here. One more major change and some fixups still due on prior changes and the number of Coverity warnings goes below 50 of which 35 are related to the sdt macros (started at 297). John Ferlan (6): nodeinfo: Use sa_assert() instead of Coverity error tag tests: Remove VIR_FREE() on static/stack buffer (der.data) lxc_driver: Need to check for vm before calling virDomainUnlock(vm) lxc_process: Avoid passing NULL iface->iname rpc: Revert Coverity tag message virbitmaptest: Resolve Coverity errors src/lxc/lxc_driver.c | 3 ++- src/lxc/lxc_process.c | 14 ++++++++------ src/nodeinfo.c | 4 ++-- src/rpc/virnetclient.c | 1 - tests/virbitmaptest.c | 24 ++++++++++++++++-------- tests/virnettlscontexttest.c | 3 --- 6 files changed, 28 insertions(+), 21 deletions(-) -- 1.7.11.7
--- src/nodeinfo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index a05159c..2cfd16c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1124,8 +1124,8 @@ nodeSetMemoryParameterValue(virTypedParameterPtr param) int ret = -1; int rc = -1; - /* coverity[returned_null] */ char *field = strchr(param->field, '_'); + sa_assert(field); field++; if (virAsprintf(&path, "%s/%s", SYSFS_MEMORY_SHARED_PATH, field) < 0) { @@ -1162,8 +1162,8 @@ nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; - /* coverity[returned_null] */ char *field = strchr(param->field, '_'); + sa_assert(field); field++; if (virAsprintf(&path, "%s/%s", SYSFS_MEMORY_SHARED_PATH, field) < 0) { -- 1.7.11.7
--- tests/virnettlscontexttest.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/virnettlscontexttest.c b/tests/virnettlscontexttest.c index 49462df..e0a2788 100644 --- a/tests/virnettlscontexttest.c +++ b/tests/virnettlscontexttest.c @@ -297,7 +297,6 @@ testTLSGenerateCert(struct testTLSCertReq *req) abort(); } asn1_delete_structure(&ext); - VIR_FREE(der.data); } /* @@ -324,7 +323,6 @@ testTLSGenerateCert(struct testTLSCertReq *req) abort(); } asn1_delete_structure(&ext); - VIR_FREE(der.data); } /* @@ -355,7 +353,6 @@ testTLSGenerateCert(struct testTLSCertReq *req) abort(); } asn1_delete_structure(&ext); - VIR_FREE(der.data); } /* -- 1.7.11.7
--- src/lxc/lxc_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 602f8ff..1fe8039 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4513,7 +4513,8 @@ static int lxcDomainOpenNamespace(virDomainPtr dom, ret = nfds; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } -- 1.7.11.7
A followon to commit id: 68dceb635 - if iface->iname is NULL, then neither virNetDevOpenvswitchRemovePort() nor virNetDevVethDelete() should be called. Found by Coverity. --- src/lxc/lxc_process.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4ad7ba0..5598a86 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -262,13 +262,15 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr iface = vm->def->nets[i]; vport = virDomainNetGetActualVirtPortProfile(iface); - if (iface->ifname) + if (iface->ifname) { ignore_value(virNetDevSetOnline(iface->ifname, false)); - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(iface), - iface->ifname)); - ignore_value(virNetDevVethDelete(iface->ifname)); + if (vport && + vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) + ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(iface), + iface->ifname)); + ignore_value(virNetDevVethDelete(iface->ifname)); + } networkReleaseActualDevice(iface); } -- 1.7.11.7
On 01/22/13 23:09, John Ferlan wrote:
A followon to commit id: 68dceb635 - if iface->iname is NULL, then neither virNetDevOpenvswitchRemovePort() nor virNetDevVethDelete() should be called. Found by Coverity. --- src/lxc/lxc_process.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
ACK. Peter
Turns out the fix for VIR_FREE made this particular Coverity tag unnecessary, so I'm removing it. --- src/rpc/virnetclient.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 7550968..44638e2 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -430,7 +430,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, VIR_FREE(privkey); /* DSA */ if (!privkey) { - /* coverity[dead_error_begin] */ virBufferAsprintf(&buf, "%s/.ssh/id_dsa", homedir); if (!(privkey = virBufferContentAndReset(&buf))) goto no_memory; -- 1.7.11.7
On 01/22/13 23:09, John Ferlan wrote:
Turns out the fix for VIR_FREE made this particular Coverity tag unnecessary, so I'm removing it. --- src/rpc/virnetclient.c | 1 - 1 file changed, 1 deletion(-)
ACK. Peter
test1: Need to check for bitmap before using as well as free it properly test2: need to check for bitsString2 before using it. --- tests/virbitmaptest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index db76672..66ffd1b 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -29,26 +29,33 @@ static int test1(const void *data ATTRIBUTE_UNUSED) int size; int bit; bool result; + int ret = -1; size = 1024; bit = 100; - bitmap = virBitmapNew(size); + if (!(bitmap = virBitmapNew(size))) + goto error; + if (virBitmapSetBit(bitmap, bit) < 0) - return -1; + goto error; if (virBitmapGetBit(bitmap, bit, &result) < 0) - return -1; + goto error; if (!result) - return -1; + goto error; if (virBitmapGetBit(bitmap, bit + 1, &result) < 0) - return -1; + goto error; if (result) - return -1; + goto error; - return 0; + ret = 0; + +error: + virBitmapFree(bitmap); + return ret; } static int @@ -102,7 +109,8 @@ static int test2(const void *data ATTRIBUTE_UNUSED) if (virBitmapCountBits(bitmap) != 48) goto error; - bitsString2 = virBitmapFormat(bitmap); + if (!(bitsString2 = virBitmapFormat(bitmap))) + goto error; if (strcmp(bitsString1, bitsString2)) goto error; -- 1.7.11.7
On 01/22/13 23:09, John Ferlan wrote:
test1: Need to check for bitmap before using as well as free it properly test2: need to check for bitsString2 before using it. --- tests/virbitmaptest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index db76672..66ffd1b 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -29,26 +29,33 @@ static int test1(const void *data ATTRIBUTE_UNUSED)
[...]
- return 0; + ret = 0; + +error:
Hm, this label would be probably better to be named "cleanup" as the success path takes it too, but that's a small nit.
+ virBitmapFree(bitmap); + return ret; }
static int
ACK, and I will push the series soon. Peter
On 01/22/2013 03:09 PM, John Ferlan wrote:
Like a gift that keeps on giving... More Coverity changes.
Adjust changes from earlier based on Eric's comments/feedback.
nodeinfo.c - use sa_assert() instead of comment: https://www.redhat.com/archives/libvir-list/2013-January/msg01578.html
virnetclient.c - remove change - the VIR_FREE change resolves this: https://www.redhat.com/archives/libvir-list/2013-January/msg01581.html
Two new Coverity warnings popped up today (lxc_driver.c & lxc_process.c). No rhyme or reason, they just were new with no recent change to blame (very strange).
Not entirely - if I understand it, Coverity tries to do some duplicate and priority suppression to avoid flooding the report with the same minor error over and over again. Based on whatever heuristics it is using, some errors won't show up in reports until enough other errors have been cleared to force the remaining problems to no longer be in the noise. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa