[libvirt] [PATCH 0/6] Coverity fixes
Some Coverity patches - the first one has been showing up in my dailies for a few days... The other 5 I don't see, but that's because I don't generally build with xenapi in my Coverity environment. So for those, I'm hoping someone with that environment could check them out. John Ferlan (6): conf: Resolve Coverity RESOURCE_LEAK xenapi: Resolve Coverity FORWARD_NULL xenapi: Resolve Coverity NO_EFFECT xenapi: Resolve Coverity NULL_RETURNS xenapi: Resolve Coverity REVERSE_INULL xenapi: Resolve Coverity REVERSE_INULL src/conf/node_device_conf.c | 1 + src/xenapi/xenapi_driver.c | 13 ++++++++----- src/xenapi/xenapi_utils.c | 33 ++++++++++++++++++--------------- 3 files changed, 27 insertions(+), 20 deletions(-) -- 2.1.0
Commit id 'c9027d8f' added parsing of the CapNet for offload SRIOV NIC discovery, but forgot to free the nodes Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index f9c9b6f..d309bc6 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1002,6 +1002,7 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, ret = 0; out: ctxt->node = orignode; + VIR_FREE(nodes); return ret; } -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:24 -0400, John Ferlan wrote:
Commit id 'c9027d8f' added parsing of the CapNet for offload SRIOV NIC discovery, but forgot to free the nodes
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index f9c9b6f..d309bc6 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1002,6 +1002,7 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, ret = 0;
The same patch also added code that uses the @tmp variable to extract a string via virXMLPropString and it is not freed either.
out: ctxt->node = orignode; + VIR_FREE(nodes); return ret; }
ACK with both leaks fixed. Peter
Since inception. Coverity complains that the code checks "(record == NULL && !session->ok)", but doesn't check (record != NULL) before dereferencing at "record->is_a_template" Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index afb6d6c..821e9d9 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1646,9 +1646,11 @@ xenapiConnectNumOfDefinedDomains(virConnectPtr conn) xen_vm_set_free(result); return -1; } - if (record->is_a_template == 0) - DomNum++; - xen_vm_record_free(record); + if (record) { + if (record->is_a_template == 0) + DomNum++; + xen_vm_record_free(record); + } } xen_vm_set_free(result); return DomNum; -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:25 -0400, John Ferlan wrote:
Since inception. Coverity complains that the code checks "(record == NULL && !session->ok)", but doesn't check (record != NULL) before dereferencing at "record->is_a_template"
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
ACK, Peter
Coverity points out that check (def->uuid) has no effect since it's not a pointer, rather an array of characters. Just remove the extranous check. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_utils.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 91bc649..ce09dfe 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -455,11 +455,9 @@ createVMRecordFromXml(virConnectPtr conn, virDomainDefPtr def, *record = xen_vm_record_alloc(); if (VIR_STRDUP((*record)->name_label, def->name) < 0) goto error; - if (def->uuid) { - virUUIDFormat(def->uuid, uuidStr); - if (VIR_STRDUP((*record)->uuid, uuidStr) < 0) - goto error; - } + virUUIDFormat(def->uuid, uuidStr); + if (VIR_STRDUP((*record)->uuid, uuidStr) < 0) + goto error; if (STREQ(def->os.type, "hvm")) { char *boot_order = NULL; if (VIR_STRDUP((*record)->hvm_boot_policy, "BIOS order") < 0) -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:26 -0400, John Ferlan wrote:
Coverity points out that check (def->uuid) has no effect since it's not a pointer, rather an array of characters. Just remove the extranous check.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_utils.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
ACK, Peter
Coverity points out that the return from virDomainDefParseString is not checked in xenapiDomainCreateXML like it should be which could end up in a pointer dereference Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 821e9d9..148ff9b 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -560,6 +560,8 @@ xenapiDomainCreateXML(virConnectPtr conn, priv->caps, priv->xmlopt, 1 << VIR_DOMAIN_VIRT_XEN, parse_flags); + if (!defPtr) + return NULL; createVMRecordFromXml(conn, defPtr, &record, &vm); virDomainDefFree(defPtr); if (record) { -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:27 -0400, John Ferlan wrote:
Coverity points out that the return from virDomainDefParseString is not checked in xenapiDomainCreateXML like it should be which could end up in a pointer dereference
NULL pointer deref.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 2 ++ 1 file changed, 2 insertions(+)
ACK, Peter
Coverity complains that "net_set" is compared to NULL before calling xen_network_set_free, but used rather liberally before that. While I was looking at the code I also noted that if the virAsprintfQuiet fails, then we leak our structures - so I added those too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_utils.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index ce09dfe..0c13745 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -399,14 +399,14 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, xen_network_set *net_set = NULL; xen_network_record *net_rec = NULL; int cnt = 0; - if (xen_network_get_all(session, &net_set)) { - for (cnt = 0; cnt < net_set->size; cnt++) { - if (xen_network_get_record(session, &net_rec, net_set->contents[cnt])) { - if (STREQ(net_rec->bridge, bridge)) { - break; - } else { - xen_network_record_free(net_rec); - } + if (!xen_network_get_all(session, &net_set)) + return -1 + for (cnt = 0; cnt < net_set->size; cnt++) { + if (xen_network_get_record(session, &net_rec, net_set->contents[cnt])) { + if (STREQ(net_rec->bridge, bridge)) { + break; + } else { + xen_network_record_free(net_rec); } } } @@ -425,8 +425,13 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, vif_record->other_config = xen_string_string_map_alloc(0); vif_record->runtime_properties = xen_string_string_map_alloc(0); vif_record->qos_algorithm_params = xen_string_string_map_alloc(0); - if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0) + if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0) { + xen_vif_free(vif); + xen_vif_record_free(vif_record); + xen_network_record_free(net_rec); + xen_network_set_free(net_set); return -1; + } xen_vif_create(session, &vif, vif_record); if (!vif) { xen_vif_free(vif); @@ -438,7 +443,7 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, xen_vif_record_free(vif_record); xen_network_record_free(net_rec); } - if (net_set != NULL) xen_network_set_free(net_set); + xen_network_set_free(net_set); return -1; } -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:28 -0400, John Ferlan wrote:
Coverity complains that "net_set" is compared to NULL before calling xen_network_set_free, but used rather liberally before that. While I was looking at the code I also noted that if the virAsprintfQuiet fails, then we leak our structures - so I added those too.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_utils.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index ce09dfe..0c13745 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -399,14 +399,14 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, xen_network_set *net_set = NULL; xen_network_record *net_rec = NULL; int cnt = 0; - if (xen_network_get_all(session, &net_set)) { - for (cnt = 0; cnt < net_set->size; cnt++) { - if (xen_network_get_record(session, &net_rec, net_set->contents[cnt])) { - if (STREQ(net_rec->bridge, bridge)) { - break; - } else { - xen_network_record_free(net_rec); - } + if (!xen_network_get_all(session, &net_set)) + return -1
missing semicolon? Also @vm_opt is allocated in the declarations section and would be leaked here. Pre-existing though, so I won't insist on fixing that too.
+ for (cnt = 0; cnt < net_set->size; cnt++) { + if (xen_network_get_record(session, &net_rec, net_set->contents[cnt])) { + if (STREQ(net_rec->bridge, bridge)) { + break; + } else { + xen_network_record_free(net_rec); } } } @@ -425,8 +425,13 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, vif_record->other_config = xen_string_string_map_alloc(0); vif_record->runtime_properties = xen_string_string_map_alloc(0); vif_record->qos_algorithm_params = xen_string_string_map_alloc(0); - if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0) + if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0) { + xen_vif_free(vif);
@vif is allocated few lines below so no need to free it yet.
+ xen_vif_record_free(vif_record); + xen_network_record_free(net_rec); + xen_network_set_free(net_set); return -1; + } xen_vif_create(session, &vif, vif_record); if (!vif) { xen_vif_free(vif); @@ -438,7 +443,7 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int device, xen_vif_record_free(vif_record); xen_network_record_free(net_rec); } - if (net_set != NULL) xen_network_set_free(net_set); + xen_network_set_free(net_set); return -1; }
Looks good, ACK if you compile test this patch before pushing, with or without fixing of the other existing leak. Peter
Coverity notes in xenapiDomainGetXMLDesc that 'vms' is dereferenced a few times before a "if (vms) xen_vm_set_free(vms);" call is made. Since we'd exit out much sooner if the fetch of the vms failed, just remove the unnecessary "if (vms)" check. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 148ff9b..8eb8d73 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1563,8 +1563,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) } xen_vif_set_free(vif_set); } - if (vms) - xen_vm_set_free(vms); + xen_vm_set_free(vms); xml = virDomainDefFormat(defPtr, flags); virDomainDefFree(defPtr); return xml; -- 2.1.0
On Tue, Mar 10, 2015 at 19:20:29 -0400, John Ferlan wrote:
Coverity notes in xenapiDomainGetXMLDesc that 'vms' is dereferenced a few times before a "if (vms) xen_vm_set_free(vms);" call is made. Since we'd exit out much sooner if the fetch of the vms failed, just remove the unnecessary "if (vms)" check.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK, Peter
On 03/10/2015 07:20 PM, John Ferlan wrote:
Some Coverity patches - the first one has been showing up in my dailies for a few days... The other 5 I don't see, but that's because I don't generally build with xenapi in my Coverity environment. So for those, I'm hoping someone with that environment could check them out.
John Ferlan (6): conf: Resolve Coverity RESOURCE_LEAK xenapi: Resolve Coverity FORWARD_NULL xenapi: Resolve Coverity NO_EFFECT xenapi: Resolve Coverity NULL_RETURNS xenapi: Resolve Coverity REVERSE_INULL xenapi: Resolve Coverity REVERSE_INULL
src/conf/node_device_conf.c | 1 + src/xenapi/xenapi_driver.c | 13 ++++++++----- src/xenapi/xenapi_utils.c | 33 ++++++++++++++++++--------------- 3 files changed, 27 insertions(+), 20 deletions(-)
Patch 1: Interesting that Coverity didn't pick that up. It usually does especially since there is a VIR_FREE(tmp) after the second call to virXMLPropString... The first one is made worse by it being a for loop too... NOTE: I also "tmp = NULL" and VIR_FREE(tmp) after cleanup Patch 5: I only paid attention too late that my build wasn't actually building xenapi... Fixed the ':' reference and the overly aggressive free(vif). Since I was there, I added a xen_vm_record_opt_free(vm_opt) in the return -1;... It does seem that vm_opt gets "absorbed" into vif_record eventually which does have free paths, so I also assumed it would be then properly freed I was able to figure out/find the libxenserver and libxenserver-devel install rpm's, built, and ran my coverity checker w/ the changes. Now pushed. Tks, John
participants (2)
-
John Ferlan -
Peter Krempa