[PATCH 00/19] Resolve Coverity warnings

[PATCH] Update GPL

[PATCH] Improve support of nested...

John Ferlan

Thursday, 16 May 2013 Thu, 16 May '13

9:57 a.m.

The following set of patches resolve a number of Coverity errors/warnings that have crept into the code. After these changes there will be zero warnings John Ferlan (19): Coverity: Resolve BAD_COMPARE - ActivateFilter() Coverity: Resolve CHECKED_RETURN - _generic_infostore_open() Coverity: Resolve CHECKED_RETURN - filter_by_pool() Coverity: Resolve CHECKED_RETURN - get_dev_from_pool Coverity: Resolve CHECKED_RETURN - get_pools() Coverity: Resolve CHECKED_RETURN - mem_rasd_to_vdev() Coverity: Resolve CHECKED_RETURN - return_enum_rasds() Coverity: Resolve DEADCODE - do_parse() Coverity: Resolve DEADCODE - get_hypervisor_enabled() Coverity: Resolve DEADCODE - octets_from_ip() Coverity: Resolve NO_EFFECT - set_proc_rasd_params() Coverity: Resolve NO_EFFECT - _set_fv_prop() Coverity: Resolve RESOURCE_LEAK - parse_os() Coverity: Resolve REVERSE_INULL - doms_to_xml() Coverity: Resolve REVERSE_INULL - lifecycle_thread_native() Coverity: Resolve UNINIT - vsss_delete_snapshot() Coverity: Resolve UNUSED_VALUE - system_xml() && mem_xml() Coverity: Resolve USE_AFTER_FREE - lifecycle_thread_native() Coverity: Resolve ARRAY_VS_SINGLETON - get_dev_paths() and callers libxkutil/device_parsing.c | 8 +++++++- libxkutil/infostore.c | 5 ++++- libxkutil/misc_util.c | 4 ++-- libxkutil/xmlgen.c | 16 ++++++++++++++++ src/Virt_ComputerSystemIndication.c | 19 +++++++++++++------ src/Virt_ElementAllocatedFromPool.c | 12 +++++++----- src/Virt_FilterEntry.c | 3 --- src/Virt_RASD.c | 20 ++++++++++++++------ src/Virt_ResourceAllocationFromPool.c | 4 +++- src/Virt_ResourcePoolConfigurationService.c | 20 +++++++------------- src/Virt_VSSD.c | 2 +- src/Virt_VirtualSystemManagementService.c | 4 +++- src/Virt_VirtualSystemSnapshotService.c | 2 +- 13 files changed, 78 insertions(+), 41 deletions(-) -- 1.8.1.4

Show replies by date

John Ferlan

Thursday, 16 May Thu, 16 May

9:57 a.m.

New subject: [PATCH 01/19] Coverity: Resolve BAD_COMPARE - ActivateFilter()

Two examples of: 807 thread->id = _BROKER->xft->newThread(lifecycle_thread_native, 808 thread, 0); 809 (1) Event null_misuse: Comparing pointer "thread->id" against NULL using anything besides == or != is likely to be incorrect. 810 if (thread->id <= 0) { 811 CU_DEBUG("Error, failed to create new thread."); 812 error = true; Resolve this and subsequent use by changing comparison to: if (thread->id == NULL) { --- src/Virt_ComputerSystemIndication.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 247143c..e9fa0c2 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -807,7 +807,7 @@ static CMPIStatus ActivateFilter(CMPIIndicationMI *mi, thread->id = _BROKER->xft->newThread(lifecycle_thread_native, thread, 0); - if (thread->id <= 0) { + if (thread->id == NULL) { CU_DEBUG("Error, failed to create new thread."); error = true; } @@ -1463,7 +1463,7 @@ static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, thread->args = args; thread->id = _BROKER->xft->newThread(lifecycle_thread, thread, 0); - if (thread->id <= 0) { + if (thread->id == NULL) { CU_DEBUG("Error, failed to create new thread."); error = true; } -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 02/19] Coverity: Resolve CHECKED_RETURN - _generic_infostore_open()

(7) Event check_return: Calling function "fstat(isc->fd, &s)" without checking return value. This library function may fail and return an error code. 196 fstat(isc->fd, &s); Resolve by adding if (fstat->isc->fd, &s) < 0) --- libxkutil/infostore.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libxkutil/infostore.c b/libxkutil/infostore.c index dd1e38c..a88b586 100644 --- a/libxkutil/infostore.c +++ b/libxkutil/infostore.c @@ -193,7 +193,10 @@ static struct infostore_ctx *_generic_infostore_open(char *filename) goto err; } - fstat(isc->fd, &s); + if (fstat(isc->fd, &s) < 0) { + CU_DEBUG("Failed to fstat infostore"); + goto err; + } if (s.st_size == 0) isc->doc = new_xml(); else -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 03/19] Coverity: Resolve CHECKED_RETURN - filter_by_pool()

115 (17) Event check_return: Calling function "cu_get_str_prop(CMPIInstance const *, char const *, char const **)" without checking return value (as is done elsewhere 72 out of 77 times). (24) Event unchecked_value: No check of the return value of "cu_get_str_prop(inst, "InstanceID", &rasd_id)". 116 cu_get_str_prop(inst, "InstanceID", &rasd_id); Resolved by adding a check against CMPI_RC_OK, if not equal then just continue --- src/Virt_ResourceAllocationFromPool.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Virt_ResourceAllocationFromPool.c b/src/Virt_ResourceAllocationFromPool.c index e702387..398eef5 100644 --- a/src/Virt_ResourceAllocationFromPool.c +++ b/src/Virt_ResourceAllocationFromPool.c @@ -113,7 +113,9 @@ static int filter_by_pool(struct inst_list *dest, CMPI_RC_OK) continue; - cu_get_str_prop(inst, "InstanceID", &rasd_id); + if (cu_get_str_prop(inst, "InstanceID", &rasd_id) != + CMPI_RC_OK) + continue; poolid = pool_member_of(_BROKER, CLASSNAME(op), type, rasd_id); if ((poolid != NULL) && STREQ(poolid, _poolid)) -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 04/19] Coverity: Resolve CHECKED_RETURN - get_dev_from_pool

(4) Event check_return: Calling function "cu_get_str_prop(CMPIInstance const *, char const *, char const **)" without checking return value (as is done elsewhere 72 out of 77 times). (11) Event unchecked_value: No check of the return value of "cu_get_str_prop(inst, "CreationClassName", &cn)". 118 cu_get_str_prop(inst, "CreationClassName", &cn); 119 cu_get_str_prop(inst, "DeviceID", &dev_id); 120 121 if ((dev_id == NULL) || (cn == NULL)) 122 continue; Resolve by making proper check of function return vs. CMPI_RC_OK and removing the NULL checks since they are redundant. --- src/Virt_ElementAllocatedFromPool.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Virt_ElementAllocatedFromPool.c b/src/Virt_ElementAllocatedFromPool.c index ad9134c..c7813b7 100644 --- a/src/Virt_ElementAllocatedFromPool.c +++ b/src/Virt_ElementAllocatedFromPool.c @@ -115,10 +115,10 @@ static CMPIStatus get_dev_from_pool(const CMPIObjectPath *ref, const char *cn = NULL; const char *dev_id = NULL; - cu_get_str_prop(inst, "CreationClassName", &cn); - cu_get_str_prop(inst, "DeviceID", &dev_id); - - if ((dev_id == NULL) || (cn == NULL)) + if (cu_get_str_prop(inst, "CreationClassName", &cn) != + CMPI_RC_OK) + continue; + if (cu_get_str_prop(inst, "DeviceID", &dev_id) != CMPI_RC_OK) continue; poolid = pool_member_of(_BROKER, cn, type, dev_id); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 05/19] Coverity: Resolve CHECKED_RETURN - get_pools()

(7) Event check_return: Calling function "cu_get_str_prop(CMPIInstance const *, char const *, char const **)" without checking return value (as is done elsewhere 72 out of 77 times). (14) Event unchecked_value: No check of the return value of "cu_get_str_prop(inst, "InstanceID", &id)". 171 cu_get_str_prop(inst, "InstanceID", &id); Resolve by checking return status vs. CMPI_RC_OK and just continuing to next entry --- src/Virt_ElementAllocatedFromPool.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Virt_ElementAllocatedFromPool.c b/src/Virt_ElementAllocatedFromPool.c index c7813b7..946580c 100644 --- a/src/Virt_ElementAllocatedFromPool.c +++ b/src/Virt_ElementAllocatedFromPool.c @@ -168,7 +168,9 @@ static CMPIStatus get_pools(const CMPIObjectPath *ref, CMPIInstance *inst = tmp.list[i]; const char *id = NULL; - cu_get_str_prop(inst, "InstanceID", &id); + if (cu_get_str_prop(inst, "InstanceID", &id) != + CMPI_RC_OK) + continue; if (!STREQC(id, poolid)) inst_list_add(list, inst); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 06/19] Coverity: Resolve CHECKED_RETURN - mem_rasd_to_vdev()

1089 dev->dev.mem.maxsize = dev->dev.mem.size; (4) Event check_return: Calling function "cu_get_u64_prop(CMPIInstance const *, char const *, uint64_t *)" without checking return value (as is done elsewhere 9 out of 10 times). (14) Event unchecked_value: No check of the return value of "cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize)". 1090 cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize); Resolve by adding check and returning a message indicating what's missing --- src/Virt_VirtualSystemManagementService.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index cbb646d..ebf3e4a 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1087,7 +1087,9 @@ static const char *mem_rasd_to_vdev(CMPIInstance *inst, return "Missing `VirtualQuantity' field in Memory RASD"; dev->dev.mem.maxsize = dev->dev.mem.size; - cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize); + ret = cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize); + if (ret != CMPI_RC_OK) + return "Missing `Limit' field in Memory RASD"; if (cu_get_str_prop(inst, "AllocationUnits", &units) != CMPI_RC_OK) { CU_DEBUG("Memory RASD has no units, assuming bytes"); -- 1.8.1.4

John Ferlan

Wednesday, 22 May Wed, 22 May

1:20 p.m.

New subject: [PATCH 06/19] Coverity: Resolve CHECKED_RETURN - mem_rasd_to_vdev()

On 05/16/2013 10:57 AM, John Ferlan wrote:

...

An issue was found with the above code since Limit doesn't seem to be a required value, thus the code has been changed to the following: diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemM index cbb646d..f1441dc 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1086,8 +1086,9 @@ static const char *mem_rasd_to_vdev(CMPIInstance *inst, if (ret != CMPI_RC_OK) return "Missing `VirtualQuantity' field in Memory RASD"; - dev->dev.mem.maxsize = dev->dev.mem.size; - cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize); + ret = cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize); + if (ret != CMPI_RC_OK) + dev->dev.mem.maxsize = dev->dev.mem.size; if (cu_get_str_prop(inst, "AllocationUnits", &units) != CMPI_RC_OK) { CU_DEBUG("Memory RASD has no units, assuming bytes"); The difference being setting mem.maxsize to mem.size only if the call fails and not messaging if it does fail.

John Ferlan

Thursday, 16 May Thu, 16 May

9:57 a.m.

New subject: [PATCH 07/19] Coverity: Resolve CHECKED_RETURN - return_enum_rasds()

1097 inst_list_init(&list); 1098 (1) Event check_return: Calling function "res_type_from_rasd_classname(char const *, uint16_t *)" without checking return value (as is done elsewhere 11 out of 12 times). (7) Event unchecked_value: No check of the return value of "res_type_from_rasd_classname( (char *)(*ref->ft->getClassName)(ref, NULL)->hdl, &type)". 1099 res_type_from_rasd_classname(CLASSNAME(ref), &type); Resolve by checking return vs. CMPI_RC_OK, setting error, and returning --- src/Virt_RASD.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index 6e8a244..ad1a2e7 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -1096,7 +1096,12 @@ static CMPIStatus return_enum_rasds(const CMPIObjectPath *ref, inst_list_init(&list); - res_type_from_rasd_classname(CLASSNAME(ref), &type); + if (res_type_from_rasd_classname(CLASSNAME(ref), &type) != CMPI_RC_OK) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to determine RASD type"); + goto out; + } s = enum_rasds(_BROKER, ref, NULL, type, properties, &list); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 08/19] Coverity: Resolve DEADCODE - do_parse()

747 if (nsv == NULL) 748 goto out; 749 750 dev_nodes = nsv->nodeTab; (2) Event notnull: At condition "nsv", the value of "nsv" cannot be NULL. (3) Event dead_error_condition: The condition "nsv" must be true. (4) Event dead_error_line: Execution cannot reach this expression "0" inside statement "count = (nsv ? nsv->nodeNr ...". 751 count = nsv ? nsv->nodeNr : 0; Resolve by not checking nsv and moving up a line to right after the nsv == NULL check --- libxkutil/device_parsing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 264d4cc..edd8bc4 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -746,9 +746,9 @@ static int do_parse(xmlNodeSet *nsv, dev_parse_func_t do_real_parse, if (nsv == NULL) goto out; + count = nsv->nodeNr; dev_nodes = nsv->nodeTab; - count = nsv ? nsv->nodeNr : 0; if (count <= 0) goto out; -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 09/19] Coverity: Resolve DEADCODE - get_hypervisor_enabled()

(1) Event assignment: Assigning: "h" = "&hypervisor_list[0]". (2) Event notnull: At condition "h != NULL", the value of "h" cannot be NULL. (3) Event dead_error_condition: The condition "h != NULL" must be true. 75 for (h = &hypervisor_list[0]; h != NULL; h++) { 76 if (strncasecmp(hypervisor, h->name, strlen(h->name)) == 0) { 77 return h->enabled; 78 } 79 } 80 (4) Event dead_error_line: Execution cannot reach this statement "return false;". Resolve by making proper endloop comparison of "h->name != NULL" --- libxkutil/misc_util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 00eb4b1..819cb71 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -72,7 +72,7 @@ static bool get_hypervisor_enabled(const char *hypervisor) { hypervisor_status_t *h; - for (h = &hypervisor_list[0]; h != NULL; h++) { + for (h = &hypervisor_list[0]; h->name != NULL; h++) { if (strncasecmp(hypervisor, h->name, strlen(h->name)) == 0) { return h->enabled; } @@ -85,7 +85,7 @@ static void set_hypervisor_disabled(const char *hypervisor) { hypervisor_status_t *h; - for (h = &hypervisor_list[0]; h != NULL; h++) { + for (h = &hypervisor_list[0]; h->name != NULL; h++) { if (strncasecmp(hypervisor, h->name, strlen(h->name)) == 0) { CU_DEBUG("Setting '%s' hypervisor as DISABLED", h->name); h->enabled = false; -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 10/19] Coverity: Resolve DEADCODE - octets_from_ip()

95 (3) Event cond_at_least: Condition "size < 4U", taking false branch. Now the value of "size" is at least 4. 96 if ((s == 0) || (s[0] == '\0') || (buffer == NULL) || (size < 4)) ... 106 family = strstr(s, ":") ? AF_INET6 : AF_INET; (1) Event assignment: Assigning: "n" = "(family == 10U) ? 16 : 4". 107 n = family == AF_INET6 ? 16 : 4; 108 (4) Event cond_at_least: Condition "size < n", taking false branch. Now the value of "size" is at least 16. 109 if (size < n) 110 return 0; 111 112 if (inet_pton(family, s, &addr)) { (2) Event intervals: At condition "n <= size", the value of "n" must be in one of the following intervals: {[4,4], [16,16]}. (5) Event at_least: At condition "n <= size", the value of "size" must be at least 4. (6) Event dead_error_condition: The condition "n <= size" must be true. (7) Event dead_error_line: Execution cannot reach this expression "size" inside statement "n = ((n <= size) ? n : size);". 113 n = n <= size ? n : size; Resolve by removing "if (size < n) return 0;" check --- src/Virt_FilterEntry.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 3c4a3e6..2932be2 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -106,9 +106,6 @@ static int octets_from_ip(const char * s, unsigned int *buffer, family = strstr(s, ":") ? AF_INET6 : AF_INET; n = family == AF_INET6 ? 16 : 4; - if (size < n) - return 0; - if (inet_pton(family, s, &addr)) { n = n <= size ? n : size; for (i = 0; i < n; i++) -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params()

143 if (domain_online(dom)) 144 count = domain_vcpu_count(dom); 145 else 146 count = dev->dev.vcpu.quantity; 147 (1) Event unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "count >= 0UL". 148 if (count >= 0) Resolve by adjusting logic. Problem was that the active count is returned as an int with an error value of -1, while the quantity value is guaranteed to be 1 or more (see parse_vcpu_device() processing). So initialize count to zero, then only set the property if count > 0. Setting count of the active condition requires a local "active_count" and checking that to be > 0 before blindly setting it to count. Imagine 0xfffffffffffffff vcpu's! --- src/Virt_RASD.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index ad1a2e7..a4cba5b 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -124,7 +124,7 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, struct infostore_ctx *info = NULL; uint32_t weight = 0; uint64_t limit; - uint64_t count; + uint64_t count = 0; conn = connect_by_classname(broker, CLASSNAME(ref), &s); if (conn == NULL) @@ -140,12 +140,15 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, goto out; } - if (domain_online(dom)) - count = domain_vcpu_count(dom); - else + if (domain_online(dom)) { + int active_count = domain_vcpu_count(dom); + if (active_count > 0) + count = active_count; + } else { count = dev->dev.vcpu.quantity; + } - if (count >= 0) + if (count > 0) CMSetProperty(inst, "VirtualQuantity", (CMPIValue *)&count, -- 1.8.1.4

WangXu

Friday, 17 May Fri, 17 May

1:19 a.m.

New subject: [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params()

----------------------------------------

...

From: jferlan(a)redhat.com To: libvirt-cim(a)redhat.com Date: Thu, 16 May 2013 10:57:46 -0400 Subject: [Libvirt-cim] [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params() 143 if (domain_online(dom)) 144 count = domain_vcpu_count(dom); 145 else 146 count = dev->dev.vcpu.quantity; 147 (1) Event unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "count>= 0UL". 148 if (count>= 0) Resolve by adjusting logic. Problem was that the active count is returned as an int with an error value of -1, while the quantity value is guaranteed to be 1 or more (see parse_vcpu_device() processing). So initialize count to zero, then only set the property if count> 0. Setting count of the active condition requires a local "active_count" and checking that to be> 0 before blindly setting it to count. Imagine 0xfffffffffffffff vcpu's! --- src/Virt_RASD.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index ad1a2e7..a4cba5b 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -124,7 +124,7 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, struct infostore_ctx *info = NULL; uint32_t weight = 0; uint64_t limit; - uint64_t count; + uint64_t count = 0; conn = connect_by_classname(broker, CLASSNAME(ref), &s); if (conn == NULL) @@ -140,12 +140,15 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, goto out; } - if (domain_online(dom)) - count = domain_vcpu_count(dom); - else + if (domain_online(dom)) { + int active_count = domain_vcpu_count(dom); + if (active_count> 0) + count = active_count; + } else { count = dev->dev.vcpu.quantity; + }

if there was some failure happened, domain_vcpu_count would return -1. I think some code should handle this failure instead of just ignore it and skip CMSetProperty, Such as cu_statusf and goto out; My suggestion is the code like this: if (domain_online(dom)) { int active_count = domain_vcpu_count(dom); if (active_count < 0) { //count never be less than zero and maybe err code -2, -3...would //be added someday. cu_status(...); goto out; } else { count = active_count; //equal or more than zero should be OK } } else { count = dev->dev.vcpu.quantity; } Xu Wang

...

- if (count>= 0) + if (count> 0) CMSetProperty(inst, "VirtualQuantity", (CMPIValue *)&count, -- 1.8.1.4 _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

John Ferlan

8:14 a.m.

New subject: [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params()

On 05/17/2013 02:19 AM, WangXu wrote:

...

---------------------------------------- > From: jferlan(a)redhat.com > To: libvirt-cim(a)redhat.com > Date: Thu, 16 May 2013 10:57:46 -0400 > Subject: [Libvirt-cim] [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params() > > 143 if (domain_online(dom)) > 144 count = domain_vcpu_count(dom); > 145 else > 146 count = dev->dev.vcpu.quantity; > 147 > > (1) Event unsigned_compare: > This greater-than-or-equal-to-zero comparison of an unsigned value > is always true. "count>= 0UL". > > 148 if (count>= 0) > > Resolve by adjusting logic. Problem was that the active count is returned > as an int with an error value of -1, while the quantity value is guaranteed > to be 1 or more (see parse_vcpu_device() processing). So initialize count to > zero, then only set the property if count> 0. Setting count of the active > condition requires a local "active_count" and checking that to be> 0 before > blindly setting it to count. Imagine 0xfffffffffffffff vcpu's! > --- > src/Virt_RASD.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c > index ad1a2e7..a4cba5b 100644 > --- a/src/Virt_RASD.c > +++ b/src/Virt_RASD.c > @@ -124,7 +124,7 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, > struct infostore_ctx *info = NULL; > uint32_t weight = 0; > uint64_t limit; > - uint64_t count; > + uint64_t count = 0; > > conn = connect_by_classname(broker, CLASSNAME(ref), &s); > if (conn == NULL) > @@ -140,12 +140,15 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, > goto out; > } > > - if (domain_online(dom)) > - count = domain_vcpu_count(dom); > - else > + if (domain_online(dom)) { > + int active_count = domain_vcpu_count(dom); > + if (active_count> 0) > + count = active_count; > + } else { > count = dev->dev.vcpu.quantity; > + } if there was some failure happened, domain_vcpu_count would return -1. I think some code should handle this failure instead of just ignore it and skip CMSetProperty, Such as cu_statusf and goto out; My suggestion is the code like this: if (domain_online(dom)) { int active_count = domain_vcpu_count(dom); if (active_count < 0) { //count never be less than zero and maybe err code -2, -3...would //be added someday. cu_status(...); goto out; } else { count = active_count; //equal or more than zero should be OK } } else { count = dev->dev.vcpu.quantity; } Xu Wang > > - if (count>= 0) > + if (count> 0) > CMSetProperty(inst, > "VirtualQuantity", > (CMPIValue *)&count, > -- > 1.8.1.4 > > _______________________________________________ > Libvirt-cim mailing list > Libvirt-cim(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvirt-cim

I will squash in the following: diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index a4cba5b..af6a43f 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -142,8 +142,14 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *br if (domain_online(dom)) { int active_count = domain_vcpu_count(dom); - if (active_count > 0) - count = active_count; + if (active_count < 0) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to get domain `%s' vcpu count", + domain); + goto out; + } + count = active_count; } else { count = dev->dev.vcpu.quantity; } NOTE: No need for an else to if (active_count < 0) since we're jumping to out John

John Ferlan

Thursday, 16 May Thu, 16 May

9:57 a.m.

New subject: [PATCH 12/19] Coverity: Resolve NO_EFFECT - _set_fv_prop()

55 bl_ct = dominfo->os_info.fv.bootlist_ct; (1) Event unsigned_compare: This less-than-zero comparison of an unsigned value is never true. "bl_ct < 0U". 56 if (bl_ct < 0) 57 return s; Resolve by changing the comparison to == rather than <. Code inspection determines that bootlist_ct could be zero, but more than likely it's some other positive value. It is never negative. --- src/Virt_VSSD.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 24bf908..7afe2f1 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -53,7 +53,7 @@ static CMPIStatus _set_fv_prop(const CMPIBroker *broker, (CMPIValue *)&fv, CMPI_boolean); bl_ct = dominfo->os_info.fv.bootlist_ct; - if (bl_ct < 0) + if (bl_ct == 0) return s; CU_DEBUG("bootlist_ct = %d", bl_ct); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 13/19] Coverity: Resolve RESOURCE_LEAK - parse_os()

There's a loop to allocate and fill in a 'blist' array with entries that is then only saved for a specific domain type ("hvm"). If not hvm, then the blist is not freed (54) Event cond_false: Condition "strcasecmp(dominfo->os_info.fv.type, "hvm") == 0", taking false branch 1101 if (STREQC(dominfo->os_info.fv.type, "hvm")) { 1102 dominfo->os_info.fv.bootlist_ct = bl_size; 1103 dominfo->os_info.fv.bootlist = blist; (55) Event if_end: End of if statement 1104 } 1105 (56) Event leaked_storage: Variable "blist" going out of scope leaks the storage it points to. Resolve by adding code to free the memory --- libxkutil/device_parsing.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index edd8bc4..436415a 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1101,6 +1101,12 @@ static int parse_os(struct domain *dominfo, xmlNode *os) if (STREQC(dominfo->os_info.fv.type, "hvm")) { dominfo->os_info.fv.bootlist_ct = bl_size; dominfo->os_info.fv.bootlist = blist; + } else { + int i; + + for (i = 0; i < bl_size; i++) + free(blist[i]); + free(blist); } return 1; -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 14/19] Coverity: Resolve REVERSE_INULL - doms_to_xml()

(1) Event deref_ptr: Directly dereferencing pointer "dom_xml_list". 436 *dom_xml_list = calloc(dom_ptr_count, sizeof(struct dom_xml)); (2) Event check_after_deref: Null-checking "dom_xml_list" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 437 if (!dom_xml_list) { Resolve by changing check to be "if (!*dom_xml_list) {" --- src/Virt_ComputerSystemIndication.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index e9fa0c2..1ae8193 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -434,7 +434,7 @@ static CMPIStatus doms_to_xml(struct dom_xml **dom_xml_list, return s; } *dom_xml_list = calloc(dom_ptr_count, sizeof(struct dom_xml)); - if (!dom_xml_list) { + if (!*dom_xml_list) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Failed calloc %d dom_xml.", dom_ptr_count); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 15/19] Coverity: Resolve REVERSE_INULL - lifecycle_thread_native()

602 csi_thread_data_t *thread = (csi_thread_data_t *) params; (1) Event alias: Assigning: "args" = "thread->args". 603 struct ind_args *args = thread->args; (2) Event deref_ptr: Directly dereferencing pointer "args". ... (3) Event check_after_deref: Null-checking "thread->args" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 728 if (thread->args != NULL) { 729 stdi_free_ind_args(&thread->args); 730 } Resolve by changing the initialization to only set 'args', 'context', and 'prefix' if thread->args is not NULL. Each is initialized to NULL so the if prefix == NULL is still valid --- src/Virt_ComputerSystemIndication.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 1ae8193..04e4d89 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -600,9 +600,9 @@ static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) { CU_DEBUG("Entering libvirtc-cim native CSI thread."); csi_thread_data_t *thread = (csi_thread_data_t *) params; - struct ind_args *args = thread->args; - CMPIContext *context = args->context; - char *prefix = class_prefix_name(args->classname); + struct ind_args *args = NULL; + CMPIContext *context = NULL; + char *prefix = NULL; virConnectPtr conn; CMPIStatus s; int retry_time = FAIL_WAIT_TIME; @@ -614,6 +614,11 @@ static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) virDomainPtr *tmp_list = NULL; int CBAttached = 0; + if (thread->args != NULL) { + args = thread->args; + context = args->context; + prefix = class_prefix_name(args->classname); + } if (prefix == NULL) { goto init_out; } -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 16/19] Coverity: Resolve UNINIT - vsss_delete_snapshot()

(1) Event var_decl: Declaring variable "s" without initializer. 493 CMPIStatus s; ... (6) Event uninit_use: Using uninitialized value "s": field "s"."msg" is uninitialized. 512 return s; Resolve by initializing properly to CMPIStatus s = {CMPI_RC_OK, NULL}; --- src/Virt_VirtualSystemSnapshotService.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Virt_VirtualSystemSnapshotService.c b/src/Virt_VirtualSystemSnapshotService.c index 8c0889d..f4f3f86 100644 --- a/src/Virt_VirtualSystemSnapshotService.c +++ b/src/Virt_VirtualSystemSnapshotService.c @@ -490,7 +490,7 @@ static CMPIStatus create_snapshot(CMPIMethodMI *self, CMPIStatus vsss_delete_snapshot(const char *domname) { - CMPIStatus s; + CMPIStatus s = {CMPI_RC_OK, NULL}; char *path = NULL; path = vsss_get_save_path(domname); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 17/19] Coverity: Resolve UNUSED_VALUE - system_xml() && mem_xml()

Numerous instances of : (1) Event returned_pointer: Pointer "tmp" returned by "xmlNewChild(root, NULL, (xmlChar *)"currentMemory", (xmlChar *)string)" is never used. 482 tmp = xmlNewChild(root, 483 NULL, ... Resolved by adding "if (tmp == NULL) return XML_ERROR;" --- libxkutil/xmlgen.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 94fb7d3..6302b60 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -483,6 +483,8 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) NULL, BAD_CAST "currentMemory", BAD_CAST string); + if (tmp == NULL) + return XML_ERROR; free(string); tmp = NULL; @@ -650,12 +652,16 @@ static char *system_xml(xmlNodePtr root, struct domain *domain) xmlNodePtr tmp; tmp = xmlNewChild(root, NULL, BAD_CAST "name", BAD_CAST domain->name); + if (tmp == NULL) + return XML_ERROR; if (domain->bootloader) { tmp = xmlNewChild(root, NULL, BAD_CAST "bootloader", BAD_CAST domain->bootloader); + if (tmp == NULL) + return XML_ERROR; } if (domain->bootloader_args) { @@ -663,25 +669,35 @@ static char *system_xml(xmlNodePtr root, struct domain *domain) NULL, BAD_CAST "bootloader_args", BAD_CAST domain->bootloader_args); + if (tmp == NULL) + return XML_ERROR; } tmp = xmlNewChild(root, NULL, BAD_CAST "on_poweroff", BAD_CAST vssd_recovery_action_str(domain->on_poweroff)); + if (tmp == NULL) + return XML_ERROR; tmp = xmlNewChild(root, NULL, BAD_CAST "on_crash", BAD_CAST vssd_recovery_action_str(domain->on_crash)); + if (tmp == NULL) + return XML_ERROR; tmp = xmlNewChild(root, NULL, BAD_CAST "uuid", BAD_CAST domain->uuid); + if (tmp == NULL) + return XML_ERROR; if (domain->clock != NULL) { tmp = xmlNewChild(root, NULL, BAD_CAST "clock", NULL); + if (tmp == NULL) + return XML_ERROR; xmlNewProp(tmp, BAD_CAST "offset", BAD_CAST domain->clock); } -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 18/19] Coverity: Resolve USE_AFTER_FREE - lifecycle_thread_native()

Resolve 2 instances where tmp_list was used after a free, both in the same location in the loop: (22) Event pass_freed_arg: Passing freed pointer "tmp_list" as an argument to function "doms_to_xml(struct dom_xml **, virDomainPtr *, int)". 658 s = doms_to_xml(&cur_xml, tmp_list, cur_count); 659 free_domain_list(tmp_list, cur_count); (10) Event freed_arg: "free(void *)" frees "tmp_list". 660 free(tmp_list); Resolve by setting "tmp_list = NULL;" after each free. --- src/Virt_ComputerSystemIndication.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 04e4d89..086d8c4 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -638,6 +638,7 @@ static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) s = doms_to_xml(&prev_xml, tmp_list, prev_count); free_domain_list(tmp_list, prev_count); free(tmp_list); + tmp_list = NULL; if (s.rc != CMPI_RC_OK) { CU_DEBUG("doms_to_xml failed. Attempting to continue."); } @@ -657,6 +658,7 @@ static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) s = doms_to_xml(&cur_xml, tmp_list, cur_count); free_domain_list(tmp_list, cur_count); free(tmp_list); + tmp_list = NULL; if (s.rc != CMPI_RC_OK) { CU_DEBUG("doms_to_xml failed. retry in %d seconds", retry_time); -- 1.8.1.4

John Ferlan

9:57 a.m.

New subject: [PATCH 19/19] Coverity: Resolve ARRAY_VS_SINGLETON - get_dev_paths() and callers

Resolve two instances (disk_fs_or_disk_or_logical_pool() and disk_iscsi_pool()) of (1) Event address_of: Taking address with "&pool->pool_info.disk.device_paths" yields a singleton pointer. (2) Event callee_ptr_arith: Passing "&pool->pool_info.disk.device_paths" to function "get_dev_paths(CMPIInstance *, char ***, uint16_t *)" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. 201 msg = get_dev_paths(inst, 202 &pool->pool_info.disk.device_paths, 203 &pool->pool_info.disk.device_paths_ct); Resolve by changing get_dev_paths() to take "pool" as an address and then expanding the variables within the code to the specific location --- src/Virt_ResourcePoolConfigurationService.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 0c0cc06..4775e01 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -154,8 +154,7 @@ static void init_disk_pool(struct virt_pool *pool) } static char *get_dev_paths(CMPIInstance *inst, - char ***path_list, - uint16_t *count) + struct virt_pool *pool) { CMPICount i; CMPICount ct; @@ -170,11 +169,11 @@ static char *get_dev_paths(CMPIInstance *inst, if ((s.rc != CMPI_RC_OK) || (ct <= 0)) return "Unable to get DevicePaths array count"; - *path_list = calloc(ct, sizeof(char *)); - if (*path_list == NULL) + pool->pool_info.disk.device_paths = calloc(ct, sizeof(char *)); + if (pool->pool_info.disk.device_paths == NULL) return "Failed to alloc space for device paths"; - *count = ct; + pool->pool_info.disk.device_paths_ct = ct; for (i = 0; i < ct; i++) { const char *str = NULL; @@ -187,7 +186,7 @@ static char *get_dev_paths(CMPIInstance *inst, if (str == NULL) return "Unable to get value of DevicePaths element"; - *path_list[i] = strdup(str); + pool->pool_info.disk.device_paths[i] = strdup(str); } return NULL; @@ -198,10 +197,7 @@ static const char *disk_fs_or_disk_or_logical_pool(CMPIInstance *inst, { const char *msg = NULL; - msg = get_dev_paths(inst, - &pool->pool_info.disk.device_paths, - &pool->pool_info.disk.device_paths_ct); - + msg = get_dev_paths(inst, pool); /* Specifying a value for DevicePaths isn't mandatory for logical pool types. */ @@ -243,9 +239,7 @@ static const char *disk_iscsi_pool(CMPIInstance *inst, const char *val = NULL; const char *msg = NULL; - msg = get_dev_paths(inst, - &pool->pool_info.disk.device_paths, - &pool->pool_info.disk.device_paths_ct); + msg = get_dev_paths(inst, pool); if (msg != NULL) return msg; -- 1.8.1.4

John Ferlan

Thursday, 20 June Thu, 20 Jun

5:27 a.m.

On 05/16/2013 10:57 AM, John Ferlan wrote:

...

After resolving a couple of issues with the changes in 6/19 and 11/19, I have pushed these. There are zero Coverity errors using the default aggressiveness for cov-analyze. John

4202

days inactive

4237

days old

cim@lists.libvirt.org

Manage subscription

23 comments

2 participants

tags (0)

participants (2)

John Ferlan
WangXu

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

[PATCH 00/19] Resolve Coverity warnings