[libvirt] [PATCH 0/9] Address Coverity false positives
As the Coverity work winds down, I've bundled together a series of patches that only add a comment tag to the offending line of code that Coverity believes the error to be on. Each submit has a commit message indicating the reason for the tag. John Ferlan (9): xend: Address some Coverity false positives lxc: Add coverity[dead_error_begin] tag in switch stmts nodeinfo: Add coverity[dead_error_begin] and [returned_null] tags xen: Add coverity[ptr_arith] and [sign_extension] tags rpc: Add coverity[dead_error_begin] tag qemu: Add coverity[negative_returns] tag remote: Avoid coverity[leaked_storage] message. network: Add coverity[leaked_handle] to ignore error storage: Add coverity[dead_error_condition] to avoid error src/lxc/lxc_driver.c | 3 +++ src/network/bridge_driver.c | 1 + src/nodeinfo.c | 3 +++ src/qemu/qemu_hotplug.c | 1 + src/remote/remote_driver.c | 1 + src/rpc/virnetclient.c | 1 + src/storage/parthelper.c | 2 ++ src/xen/xen_hypervisor.c | 5 ++++- src/xen/xend_internal.c | 4 ++++ 9 files changed, 20 insertions(+), 1 deletion(-) -- 1.7.11.7
The various _for_i loops with both u.s.car and u.s.cdr were being reported as COPY_PASTE errors by Coverity. This just quiets those messages. --- src/xen/xend_internal.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 038dd1e..00d1c9b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2068,6 +2068,7 @@ xenDaemonListDomains(virConnectPtr conn, int *ids, int maxids) ret = 0; + /* coverity[copy_paste_error] */ for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) @@ -2105,6 +2106,7 @@ xenDaemonNumOfDomains(virConnectPtr conn) ret = 0; + /* coverity[copy_paste_error] */ for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) @@ -3431,6 +3433,7 @@ xenDaemonNumOfDefinedDomains(virConnectPtr conn) ret = 0; + /* coverity[copy_paste_error] */ for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) @@ -3464,6 +3467,7 @@ xenDaemonListDefinedDomains(virConnectPtr conn, char **const names, int maxnames ret = 0; + /* coverity[copy_paste_error] */ for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) -- 1.7.11.7
The use of switch statements inside a bounded for loop resulted in some false positives regarding the "default:" label which cannot be reached since each of the other case statements use the possible for loop values. --- src/lxc/lxc_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3268e22..70b4f52 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -920,6 +920,7 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; break; + /* coverity[dead_error_begin] */ default: break; /* should not hit here */ @@ -2175,6 +2176,7 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; break; + /* coverity[dead_error_begin] */ default: break; /* should not hit here */ @@ -2192,6 +2194,7 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; break; + /* coverity[dead_error_begin] */ default: break; /* should not hit here */ -- 1.7.11.7
The use of switch statements inside a bounded for loop resulted in some false positives regarding the "default:" label which cannot be reached since each of the other case statements use the possible for loop values. A [dead_error_begin] was added before the default label. Commit id ebdbe25a adjusted the algorithm and the caller guarantees that the 'params' will have a '_' in the name being searched. Add the [returned_null] tag to the two instances. --- src/nodeinfo.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..a05159c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1124,6 +1124,7 @@ nodeSetMemoryParameterValue(virTypedParameterPtr param) int ret = -1; int rc = -1; + /* coverity[returned_null] */ char *field = strchr(param->field, '_'); field++; if (virAsprintf(&path, "%s/%s", @@ -1161,6 +1162,7 @@ nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; + /* coverity[returned_null] */ char *field = strchr(param->field, '_'); field++; if (virAsprintf(&path, "%s/%s", @@ -1413,6 +1415,7 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, break; + /* coverity[dead_error_begin] */ default: break; } -- 1.7.11.7
On 01/22/2013 07:40 AM, John Ferlan wrote:
The use of switch statements inside a bounded for loop resulted in some false positives regarding the "default:" label which cannot be reached since each of the other case statements use the possible for loop values. A [dead_error_begin] was added before the default label.
Commit id ebdbe25a adjusted the algorithm and the caller guarantees that the 'params' will have a '_' in the name being searched. Add the [returned_null] tag to the two instances. --- src/nodeinfo.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..a05159c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1124,6 +1124,7 @@ nodeSetMemoryParameterValue(virTypedParameterPtr param) int ret = -1; int rc = -1;
+ /* coverity[returned_null] */ char *field = strchr(param->field, '_');
Another possibility is to use: char *field = strchr(param->field, '_'); sa_assert(field); which would have the advantage of working with clang in addition to Coverity, if clang also turns out to have the same false positive. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/22/2013 11:32 AM, Eric Blake wrote:
On 01/22/2013 07:40 AM, John Ferlan wrote:
The use of switch statements inside a bounded for loop resulted in some false positives regarding the "default:" label which cannot be reached since each of the other case statements use the possible for loop values. A [dead_error_begin] was added before the default label.
Commit id ebdbe25a adjusted the algorithm and the caller guarantees that the 'params' will have a '_' in the name being searched. Add the [returned_null] tag to the two instances. --- src/nodeinfo.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..a05159c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1124,6 +1124,7 @@ nodeSetMemoryParameterValue(virTypedParameterPtr param) int ret = -1; int rc = -1;
+ /* coverity[returned_null] */ char *field = strchr(param->field, '_');
Another possibility is to use:
char *field = strchr(param->field, '_'); sa_assert(field);
which would have the advantage of working with clang in addition to Coverity, if clang also turns out to have the same false positive.
That works too - I will put together a new patch John
The old cpu bitmap setting algorithm causes a couple of complaints which have been tagged. --- src/xen/xen_hypervisor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a770f53..bfee56d 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1; memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + } if (hv_versions.hypervisor == 1) { xen_op_v1 op; -- 1.7.11.7
On 01/22/2013 07:40 AM, John Ferlan wrote:
The old cpu bitmap setting algorithm causes a couple of complaints which have been tagged. --- src/xen/xen_hypervisor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a770f53..bfee56d 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1;
memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + }
Having to add two comments to shut up Coverity feels awkward. Would it also work to do 'uint64_t j' instead of the current 'int j' in the declaration a few lines earlier? Not only would it be a smaller diff, but the fewer Coverity comments we have to use, the better I feel. I know this has already been pushed, but it is still worth seeing if a followup patch can clean things further. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/22/2013 11:35 AM, Eric Blake wrote:
On 01/22/2013 07:40 AM, John Ferlan wrote:
The old cpu bitmap setting algorithm causes a couple of complaints which have been tagged. --- src/xen/xen_hypervisor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a770f53..bfee56d 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1;
memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + }
Having to add two comments to shut up Coverity feels awkward. Would it also work to do 'uint64_t j' instead of the current 'int j' in the declaration a few lines earlier? Not only would it be a smaller diff, but the fewer Coverity comments we have to use, the better I feel.
I know this has already been pushed, but it is still worth seeing if a followup patch can clean things further.
Nope, just tried using uint64_t on 'j' without any luck. I also tried putting the comments on the same line without the desired effect. Here's data on the two reported defects (I turned OFF line wrap for this - the line numbers are from an older analysis): Error: ARRAY_VS_SINGLETON (CWE-119): [#def1] libvirt-1.0.0/src/xen/xen_hypervisor.c:1751: cond_false: Condition "hv_versions.hypervisor > 1", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1790: else_branch: Reached else branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: address_of: Taking address with "&xen_cpumap" yields a singleton pointer. libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: assign: Assigning: "pm" = "&xen_cpumap". libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "maplen > 8 /* (int)sizeof (cpumap_t) */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "0UL /* sizeof (cpumap_t) & 7 */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1799: cond_true: Condition "j < maplen", taking true branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: ptr_arith: Using "pm" as an array. This might corrupt or misinterpret adjacent memory locations. AND Error: SIGN_EXTENSION (CWE-194): [#def245] libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8 * (j & 7)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "cpumap[j] << 8 * (j & 7)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1. John
On 01/22/2013 12:28 PM, John Ferlan wrote:
@@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1;
memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + }
Having to add two comments to shut up Coverity feels awkward. Would it also work to do 'uint64_t j' instead of the current 'int j' in the declaration a few lines earlier? Not only would it be a smaller diff, but the fewer Coverity comments we have to use, the better I feel.
I know this has already been pushed, but it is still worth seeing if a followup patch can clean things further.
Ouch, we really DO have a bug, not to mention some very horrible code trying to do nasty aliasing that is not very portable. I'm surprised we don't have alignment complaints, by trying to treat cpumap_t as an array of 64-bit integers.
Nope, just tried using uint64_t on 'j' without any luck. I also tried putting the comments on the same line without the desired effect. Here's data on the two reported defects (I turned OFF line wrap for this - the line numbers are from an older analysis):
Error: ARRAY_VS_SINGLETON (CWE-119): [#def1] libvirt-1.0.0/src/xen/xen_hypervisor.c:1751: cond_false: Condition "hv_versions.hypervisor > 1", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1790: else_branch: Reached else branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: address_of: Taking address with "&xen_cpumap" yields a singleton pointer. libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: assign: Assigning: "pm" = "&xen_cpumap". libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "maplen > 8 /* (int)sizeof (cpumap_t) */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "0UL /* sizeof (cpumap_t) & 7 */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1799: cond_true: Condition "j < maplen", taking true branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: ptr_arith: Using "pm" as an array. This might corrupt or misinterpret adjacent memory locations.
This one, I don't know if we can silence without a coverity comment. Basically, it boils down to whether cpumap_t is typedef'd to something that can possibly be larger than 64 bits (it isn't - Coverity just confirmed that sizeof(cpumap_t) is 8 bytes). Since we just ensured that maplen will not go beyond the bounds of a 64-bit int array that overlays the same memory space, I'm okay with the /* coverity[ptr_arith] */ comment, but see below...
AND
Error: SIGN_EXTENSION (CWE-194): [#def245] libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8 * (j & 7)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "cpumap[j] << 8 * (j & 7)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
Here is the real bug (but I'm surprised why it didn't go away when you changed j from int to int64_t). When j==4, you are attempting to do 'int << (8*4)'; but you _can't_ portably shift a 32-bit integer by any more than 31 bits. We _have_ to add in a type conversion to force this shift to occur in 64-bit math, such as: *(pm + (j / 8)) |= cpumap[j] << (8ULL * (j & 7)); Or better yet, why even futz around with 64-bit aliasing? It looks like this code is trying to take endian-independent input and force it into an endian-dependent xen_cpumap variable. I think it might be cleaner as: } else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ uint64_t val = 0; int j; if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) return -1; memset(&xen_cpumap, 0, sizeof(*xen_cpumap)); for (j = 0; j < maplen; j++) { val |= cpumap[j] << (8ULL * (j & 7)); if (j % 7 == 7) { memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); val = 0; } } and see if that shuts up Coverity. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/28/2013 06:52 PM, Eric Blake wrote:
On 01/22/2013 12:28 PM, John Ferlan wrote:
@@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1;
memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + }
Having to add two comments to shut up Coverity feels awkward. Would it also work to do 'uint64_t j' instead of the current 'int j' in the declaration a few lines earlier? Not only would it be a smaller diff, but the fewer Coverity comments we have to use, the better I feel.
I know this has already been pushed, but it is still worth seeing if a followup patch can clean things further.
Ouch, we really DO have a bug, not to mention some very horrible code trying to do nasty aliasing that is not very portable. I'm surprised we don't have alignment complaints, by trying to treat cpumap_t as an array of 64-bit integers.
Nope, just tried using uint64_t on 'j' without any luck. I also tried putting the comments on the same line without the desired effect. Here's data on the two reported defects (I turned OFF line wrap for this - the line numbers are from an older analysis):
Error: ARRAY_VS_SINGLETON (CWE-119): [#def1] libvirt-1.0.0/src/xen/xen_hypervisor.c:1751: cond_false: Condition "hv_versions.hypervisor > 1", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1790: else_branch: Reached else branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: address_of: Taking address with "&xen_cpumap" yields a singleton pointer. libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: assign: Assigning: "pm" = "&xen_cpumap". libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "maplen > 8 /* (int)sizeof (cpumap_t) */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "0UL /* sizeof (cpumap_t) & 7 */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1799: cond_true: Condition "j < maplen", taking true branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: ptr_arith: Using "pm" as an array. This might corrupt or misinterpret adjacent memory locations.
This one, I don't know if we can silence without a coverity comment. Basically, it boils down to whether cpumap_t is typedef'd to something that can possibly be larger than 64 bits (it isn't - Coverity just confirmed that sizeof(cpumap_t) is 8 bytes). Since we just ensured that maplen will not go beyond the bounds of a 64-bit int array that overlays the same memory space, I'm okay with the /* coverity[ptr_arith] */ comment, but see below...
AND
Error: SIGN_EXTENSION (CWE-194): [#def245] libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8 * (j & 7)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "cpumap[j] << 8 * (j & 7)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
Here is the real bug (but I'm surprised why it didn't go away when you changed j from int to int64_t). When j==4, you are attempting to do 'int << (8*4)'; but you _can't_ portably shift a 32-bit integer by any more than 31 bits. We _have_ to add in a type conversion to force this shift to occur in 64-bit math, such as:
*(pm + (j / 8)) |= cpumap[j] << (8ULL * (j & 7));
Or better yet, why even futz around with 64-bit aliasing? It looks like this code is trying to take endian-independent input and force it into an endian-dependent xen_cpumap variable. I think it might be cleaner as:
} else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ uint64_t val = 0; int j;
if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) return -1;
memset(&xen_cpumap, 0, sizeof(*xen_cpumap)); s/*xen_cpumap/xen_cpumap
-> Compiler error
for (j = 0; j < maplen; j++) { val |= cpumap[j] << (8ULL * (j & 7)); if (j % 7 == 7) { memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); val = 0; } }
and see if that shuts up Coverity.
We get a SIGN_EXTENSION bug: (1) Event sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8ULL * (j & 7ULL)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "cpumap[j] << 8ULL * (j & 7ULL)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1. 1815 val |= cpumap[j] << (8ULL * (j & 7ULL)); I initially tried changing "j" to uint64_t and 7 to 7ULL to no avail... When I read the text, googled, which indicated that the unsigned char array cpumap will be promoted to an 'int' when using arithmetic shift, so I went the other way and changed 'val' an 'int' and the 8ULL to just 8. That removed the SIGN_EXTENSION leaving a DEADCODE error 1813 memset(&xen_cpumap, 0, sizeof(xen_cpumap)); (1) Event assignment: Assigning: "j" = "0". (2) Event incr: Incrementing "j". The value of "j" is now 1. (3) Event incr: Incrementing "j". The value of "j" is now 2. Also see events: [at_least][dead_error_condition][dead_error_begin] 1814 for (j = 0; j < maplen; j++) { 1815 val |= cpumap[j] << (8 * (j & 7)); (4) Event at_least: At condition "j % 7 == 7", the value of "j" must be at least 0. (5) Event dead_error_condition: The condition "j % 7 == 7" cannot be true. Also see events: [assignment][incr][incr][dead_error_begin] 1816 if (j % 7 == 7) { (6) Event dead_error_begin: Execution cannot reach this statement "memcpy((char *)&xen_cpumap ...". Also see events: [assignment][incr][incr][at_least][dead_error_condition] 1817 memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); 1818 val = 0; 1819 } 1820 } Changing the "j % 7 == 7" to "(j & 7) == 7" removed that condition (where the parens around j&7 were required by the compiler. Leaving the following which had no Coverity errors: cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ int val = 0; int j; if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) return -1; memset(&xen_cpumap, 0, sizeof(xen_cpumap)); for (j = 0; j < maplen; j++) { val |= cpumap[j] << (8 * (j & 7)); if ((j & 7) == 7) { memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); val = 0; } } John
On 01/28/2013 06:52 PM, Eric Blake wrote:
On 01/22/2013 12:28 PM, John Ferlan wrote:
@@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, return -1;
memset(pm, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) + for (j = 0; j < maplen; j++) { + /* coverity[ptr_arith] */ + /* coverity[sign_extension] */ *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7)); + }
Having to add two comments to shut up Coverity feels awkward. Would it also work to do 'uint64_t j' instead of the current 'int j' in the declaration a few lines earlier? Not only would it be a smaller diff, but the fewer Coverity comments we have to use, the better I feel.
I know this has already been pushed, but it is still worth seeing if a followup patch can clean things further.
Ouch, we really DO have a bug, not to mention some very horrible code trying to do nasty aliasing that is not very portable. I'm surprised we don't have alignment complaints, by trying to treat cpumap_t as an array of 64-bit integers.
Nope, just tried using uint64_t on 'j' without any luck. I also tried putting the comments on the same line without the desired effect. Here's data on the two reported defects (I turned OFF line wrap for this - the line numbers are from an older analysis):
Error: ARRAY_VS_SINGLETON (CWE-119): [#def1] libvirt-1.0.0/src/xen/xen_hypervisor.c:1751: cond_false: Condition "hv_versions.hypervisor > 1", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1790: else_branch: Reached else branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: address_of: Taking address with "&xen_cpumap" yields a singleton pointer. libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: assign: Assigning: "pm" = "&xen_cpumap". libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "maplen > 8 /* (int)sizeof (cpumap_t) */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "0UL /* sizeof (cpumap_t) & 7 */", taking false branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1799: cond_true: Condition "j < maplen", taking true branch libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: ptr_arith: Using "pm" as an array. This might corrupt or misinterpret adjacent memory locations.
This one, I don't know if we can silence without a coverity comment. Basically, it boils down to whether cpumap_t is typedef'd to something that can possibly be larger than 64 bits (it isn't - Coverity just confirmed that sizeof(cpumap_t) is 8 bytes). Since we just ensured that maplen will not go beyond the bounds of a 64-bit int array that overlays the same memory space, I'm okay with the /* coverity[ptr_arith] */ comment, but see below...
AND
Error: SIGN_EXTENSION (CWE-194): [#def245] libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8 * (j & 7)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "cpumap[j] << 8 * (j & 7)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
Here is the real bug (but I'm surprised why it didn't go away when you changed j from int to int64_t). When j==4, you are attempting to do 'int << (8*4)'; but you _can't_ portably shift a 32-bit integer by any more than 31 bits. We _have_ to add in a type conversion to force this shift to occur in 64-bit math, such as:
*(pm + (j / 8)) |= cpumap[j] << (8ULL * (j & 7));
Or better yet, why even futz around with 64-bit aliasing? It looks like this code is trying to take endian-independent input and force it into an endian-dependent xen_cpumap variable. I think it might be cleaner as:
} else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ uint64_t val = 0; int j;
if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) return -1;
memset(&xen_cpumap, 0, sizeof(*xen_cpumap)); for (j = 0; j < maplen; j++) { val |= cpumap[j] << (8ULL * (j & 7)); if (j % 7 == 7) { memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); val = 0; } }
and see if that shuts up Coverity.
FWIW: This path of code only occurs when "(hv_versions.hypervisor <= 1)" - IOW really old code. I checked history and the code was added by commit id '86247f2c'. Also since the target of the "|" operation is a 'uint64_t' (e.g. *(pm + (j / 8)), wouldn't the shift from 0->56 be OK (e.g. (8 * (j & 7)))? That is it's not an 'int << (8*4)' it's a 'uint64_t << (8*4)'. When first approaching this I figured I didn't want to introduce a bug into code that's been around a long time and that may not have any one using it. I agree the line looks ugly and it did take me a bit to think about it. Mathematically what you propose with the memcpy() works; however, I come from an architecture where a memcpy to an unaligned address causes a fault of which there'd be many. I wrote a little sample loop that went from 0->63 and printed out values just to see what one would get. The memcpy value is the newer algorithm and the *pm value is the former algorithm. j=0 j%7=0 j&7=0 memcpy=0x7fffde5b4828 *pm=0x7fffde5b4828 shift=0 j=1 j%7=1 j&7=1 memcpy=0x7fffde5b4829 *pm=0x7fffde5b4828 shift=8 j=2 j%7=2 j&7=2 memcpy=0x7fffde5b482a *pm=0x7fffde5b4828 shift=16 j=3 j%7=3 j&7=3 memcpy=0x7fffde5b482b *pm=0x7fffde5b4828 shift=24 j=4 j%7=4 j&7=4 memcpy=0x7fffde5b482c *pm=0x7fffde5b4828 shift=32 j=5 j%7=5 j&7=5 memcpy=0x7fffde5b482d *pm=0x7fffde5b4828 shift=40 j=6 j%7=6 j&7=6 memcpy=0x7fffde5b482e *pm=0x7fffde5b4828 shift=48 j=7 j%7=0 j&7=7 memcpy=0x7fffde5b482f *pm=0x7fffde5b4828 shift=56 j=8 j%7=1 j&7=0 memcpy=0x7fffde5b4830 *pm=0x7fffde5b4830 shift=0 j=9 j%7=2 j&7=1 memcpy=0x7fffde5b4831 *pm=0x7fffde5b4830 shift=8 j=10 j%7=3 j&7=2 memcpy=0x7fffde5b4832 *pm=0x7fffde5b4830 shift=16 j=11 j%7=4 j&7=3 memcpy=0x7fffde5b4833 *pm=0x7fffde5b4830 shift=24 j=12 j%7=5 j&7=4 memcpy=0x7fffde5b4834 *pm=0x7fffde5b4830 shift=32 j=13 j%7=6 j&7=5 memcpy=0x7fffde5b4835 *pm=0x7fffde5b4830 shift=40 j=14 j%7=0 j&7=6 memcpy=0x7fffde5b4836 *pm=0x7fffde5b4830 shift=48 j=15 j%7=1 j&7=7 memcpy=0x7fffde5b4837 *pm=0x7fffde5b4830 shift=56 ... j=56 j%7=0 j&7=0 memcpy=0x7fffde5b4860 *pm=0x7fffde5b4860 shift=0 j=57 j%7=1 j&7=1 memcpy=0x7fffde5b4861 *pm=0x7fffde5b4860 shift=8 j=58 j%7=2 j&7=2 memcpy=0x7fffde5b4862 *pm=0x7fffde5b4860 shift=16 j=59 j%7=3 j&7=3 memcpy=0x7fffde5b4863 *pm=0x7fffde5b4860 shift=24 j=60 j%7=4 j&7=4 memcpy=0x7fffde5b4864 *pm=0x7fffde5b4860 shift=32 j=61 j%7=5 j&7=5 memcpy=0x7fffde5b4865 *pm=0x7fffde5b4860 shift=40 j=62 j%7=6 j&7=6 memcpy=0x7fffde5b4866 *pm=0x7fffde5b4860 shift=48 j=63 j%7=0 j&7=7 memcpy=0x7fffde5b4867 *pm=0x7fffde5b4860 shift=56 John
On 01/29/2013 06:41 AM, John Ferlan wrote:
*(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7));
FWIW: This path of code only occurs when "(hv_versions.hypervisor <= 1)" - IOW really old code. I checked history and the code was added by commit id '86247f2c'. Also since the target of the "|" operation is a 'uint64_t' (e.g. *(pm + (j / 8)), wouldn't the shift from 0->56 be OK (e.g. (8 * (j & 7)))? That is it's not an 'int << (8*4)' it's a 'uint64_t << (8*4)'.
Order of operations, broken down by type: *(uint64_t* + (int / int)) |= unsigned char << (int * (int & int)) *(uint64_t* + int) |= unsigned char << int *(uint64_t*) |= int << int uint64_t = uint64_t | int There really is a sign extension bug, because 'unsigned char << int' uses 'int' math, but we then widen the int into uint64_t. If vcpu 31 is turned on, we end up sign-extending and also enabling vcpus 32-63 at the same time, which was not the goal.
When first approaching this I figured I didn't want to introduce a bug into code that's been around a long time and that may not have any one using it. I agree the line looks ugly and it did take me a bit to think about it.
Just because no one has reported the bug or tested it lately doesn't mean that the bug isn't there - it has been there a LONG time. :)
Mathematically what you propose with the memcpy() works; however, I come from an architecture where a memcpy to an unaligned address causes a fault
Such a memcpy() implementation would be flawed, and should be reported as a bug to that vendor; thankfully, these days, most libc do not have that flaw. The C standard guarantees that memcpy() is required to work on unaligned copies (for a certain definition of work; you may still end up splicing together unrelated portions of adjacent integers when reading things back as integers, but byte-for-byte, the operation is well-defined). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/29/2013 01:12 PM, Eric Blake wrote:
On 01/29/2013 06:41 AM, John Ferlan wrote:
*(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7));
FWIW: This path of code only occurs when "(hv_versions.hypervisor <= 1)" - IOW really old code. I checked history and the code was added by commit id '86247f2c'. Also since the target of the "|" operation is a 'uint64_t' (e.g. *(pm + (j / 8)), wouldn't the shift from 0->56 be OK (e.g. (8 * (j & 7)))? That is it's not an 'int << (8*4)' it's a 'uint64_t << (8*4)'.
Order of operations, broken down by type:
*(uint64_t* + (int / int)) |= unsigned char << (int * (int & int)) *(uint64_t* + int) |= unsigned char << int *(uint64_t*) |= int << int uint64_t = uint64_t | int
There really is a sign extension bug, because 'unsigned char << int' uses 'int' math, but we then widen the int into uint64_t. If vcpu 31 is turned on, we end up sign-extending and also enabling vcpus 32-63 at the same time, which was not the goal.
The following silences the [sign_extension] issue: *(pm + (j / 8)) |= (uint64_t)cpumap[j] << (8 * (j & 7)); The following silences the [ptr_arith] issue: pm = (uint64_t *)((uint64_t)&xen_cpumap + (j & ~0x7UL)); *pm |= (uint64_t)cpumap[j] << (8 * (j & 7)); Of course the code from my first response last night also silences both.
When first approaching this I figured I didn't want to introduce a bug into code that's been around a long time and that may not have any one using it. I agree the line looks ugly and it did take me a bit to think about it.
Just because no one has reported the bug or tested it lately doesn't mean that the bug isn't there - it has been there a LONG time. :)
Understood. Nobody's complained either so no one's been able to fix what's not broke :-)
Mathematically what you propose with the memcpy() works; however, I come from an architecture where a memcpy to an unaligned address causes a fault
Such a memcpy() implementation would be flawed, and should be reported as a bug to that vendor; thankfully, these days, most libc do not have that flaw. The C standard guarantees that memcpy() is required to work on unaligned copies (for a certain definition of work; you may still end up splicing together unrelated portions of adjacent integers when reading things back as integers, but byte-for-byte, the operation is well-defined).
Alpha and Itanium have some (silent unless noise enabled) heartache when the "to" address is "0x7ffe8f2b8cf1" (or x2, x3, x5, x6, x7, etc). Ending with x0 & x8 are preferred while x4 & xc tolerated. If the noise isn't turned on, the application performs poorly due to excessive alignment faults. John
On 01/29/2013 01:13 PM, John Ferlan wrote:
Mathematically what you propose with the memcpy() works; however, I come from an architecture where a memcpy to an unaligned address causes a fault
Such a memcpy() implementation would be flawed, and should be reported as a bug to that vendor; thankfully, these days, most libc do not have that flaw. The C standard guarantees that memcpy() is required to work on unaligned copies (for a certain definition of work; you may still end up splicing together unrelated portions of adjacent integers when reading things back as integers, but byte-for-byte, the operation is well-defined).
Alpha and Itanium have some (silent unless noise enabled) heartache when the "to" address is "0x7ffe8f2b8cf1" (or x2, x3, x5, x6, x7, etc). Ending with x0 & x8 are preferred while x4 & xc tolerated. If the noise isn't turned on, the application performs poorly due to excessive alignment faults.
No one's arguing that alignment doesn't matter - it is very much in the compiler's favor to align things, because performance is noticeable on unaligned copies. But the point remains that memcpy() is required to work on unaligned memory, even if the compiler does a good job at normally aligning things so that most uses of memcpy don't have to suffer speed penalties. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Coverity misses the nuance of VIR_FREE(privkey) setting privkey = NULL when if (!(virFileExists(privkey))) is true and thus declares the code dead. --- src/rpc/virnetclient.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 44638e2..7550968 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -430,6 +430,7 @@ 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/2013 07:41 AM, John Ferlan wrote:
Coverity misses the nuance of VIR_FREE(privkey) setting privkey = NULL when if (!(virFileExists(privkey))) is true and thus declares the code dead. --- src/rpc/virnetclient.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 44638e2..7550968 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -430,6 +430,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, VIR_FREE(privkey); /* DSA */ if (!privkey) { + /* coverity[dead_error_begin] */
I think a LOT of these cleanups would have been easier to evaluate if we had seen the actual Coverity trace that was being silenced. For this particular code, avoiding the Coverity comment may be as simple as reindenting things, turning the old: if (!(virFileExists(privkey))) VIR_FREE(privkey); if (!privkey) { privkey = ... ... } into: if (!virFileExists(privkey)) { VIR_FREE(privkey); privkey = ... ... } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This avoids "Event negative_returns: A negative constant "-1" is passed as an argument to a parameter that cannot be negative.". The called function uses -1 to determine whether it needs to traverse all the hostdevs. --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 19172e1..8103183 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2539,6 +2539,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, detach = vm->def->nets[detachidx]; if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* coverity[negative_returns] */ ret = qemuDomainDetachThisHostDevice(driver, vm, virDomainNetGetActualHostdev(detach), -1); -- 1.7.11.7
Upon successful return of virNetClientStreamEventAddCallback() the allocated cbdata field will be freed by virNetClientStreamEventRemoveCallback() as cbOpaque using the free function remoteStreamCallbackFree(). --- src/remote/remote_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3555dac..341321b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4816,6 +4816,7 @@ remoteStreamEventAddCallback(virStreamPtr st, cleanup: remoteDriverUnlock(priv); + /* coverity[leaked_storage] - cbdata is not leaked */ return ret; } -- 1.7.11.7
On 22.01.2013 15:41, John Ferlan wrote:
Upon successful return of virNetClientStreamEventAddCallback() the allocated cbdata field will be freed by virNetClientStreamEventRemoveCallback() as cbOpaque using the free function remoteStreamCallbackFree(). --- src/remote/remote_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3555dac..341321b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4816,6 +4816,7 @@ remoteStreamEventAddCallback(virStreamPtr st,
cleanup: remoteDriverUnlock(priv); + /* coverity[leaked_storage] - cbdata is not leaked */ return ret; }
The cleanup label is useless in the function. But it's already been there.
On error, the 'tapfd' in networkStartNetworkVirtual() is synonymous with 'macTapIfName' and will be closed in the appropriate error path. --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 268dada..21255f0 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2530,6 +2530,7 @@ networkStartNetworkVirtual(struct network_driver *driver, virSetError(save_err); virFreeError(save_err); } + /* coverity[leaked_handle] - 'tapfd' is not leaked */ return -1; } -- 1.7.11.7
The local redefinition of PED_PARTITION_PROTECTED results in the error but is not a problem especially if the built code doesn't have the latest definitions. --- src/storage/parthelper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 83f8279..c4af48f 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -129,6 +129,7 @@ int main(int argc, char **argv) content = "free"; else if (part->type & PED_PARTITION_METADATA) content = "metadata"; + /* coverity[dead_error_condition] - not true if defined */ else if (part->type & PED_PARTITION_PROTECTED) content = "protected"; else @@ -142,6 +143,7 @@ int main(int argc, char **argv) content = "free"; else if (part->type & PED_PARTITION_METADATA) content = "metadata"; + /* coverity[dead_error_condition] - not true if defined */ else if (part->type & PED_PARTITION_PROTECTED) content = "protected"; else -- 1.7.11.7
On 22.01.2013 15:40, John Ferlan wrote:
As the Coverity work winds down, I've bundled together a series of patches that only add a comment tag to the offending line of code that Coverity believes the error to be on. Each submit has a commit message indicating the reason for the tag.
John Ferlan (9): xend: Address some Coverity false positives lxc: Add coverity[dead_error_begin] tag in switch stmts nodeinfo: Add coverity[dead_error_begin] and [returned_null] tags xen: Add coverity[ptr_arith] and [sign_extension] tags rpc: Add coverity[dead_error_begin] tag qemu: Add coverity[negative_returns] tag remote: Avoid coverity[leaked_storage] message. network: Add coverity[leaked_handle] to ignore error storage: Add coverity[dead_error_condition] to avoid error
src/lxc/lxc_driver.c | 3 +++ src/network/bridge_driver.c | 1 + src/nodeinfo.c | 3 +++ src/qemu/qemu_hotplug.c | 1 + src/remote/remote_driver.c | 1 + src/rpc/virnetclient.c | 1 + src/storage/parthelper.c | 2 ++ src/xen/xen_hypervisor.c | 5 ++++- src/xen/xend_internal.c | 4 ++++ 9 files changed, 20 insertions(+), 1 deletion(-)
ACKed series and pushed. Michal
participants (3)
-
Eric Blake -
John Ferlan -
Michal Privoznik