8:40 a.m.
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
8:40 a.m.
New subject: [libvirt] [PATCH 1/9] xend: Address some Coverity false positives
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
8:40 a.m.
New subject: [libvirt] [PATCH 2/9] lxc: Add coverity[dead_error_begin] tag in switch stmts
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
8:40 a.m.
New subject: [libvirt] [PATCH 3/9] nodeinfo: Add coverity[dead_error_begin] and [returned_null] tags
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
10:32 a.m.
New subject: [libvirt] [PATCH 3/9] nodeinfo: Add coverity[dead_error_begin] and [returned_null] tags
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
1:32 p.m.
New subject: [libvirt] [PATCH 3/9] nodeinfo: Add coverity[dead_error_begin] and [returned_null] tags
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
8:40 a.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
10:35 a.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
1:28 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
5:52 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
6:55 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
7:41 a.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
12:12 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
2:13 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
2:37 p.m.
New subject: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags
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
8:41 a.m.
New subject: [libvirt] [PATCH 5/9] rpc: Add coverity[dead_error_begin] tag
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
10:39 a.m.
New subject: [libvirt] [PATCH 5/9] rpc: Add coverity[dead_error_begin] tag
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
8:41 a.m.
New subject: [libvirt] [PATCH 6/9] qemu: Add coverity[negative_returns] tag
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
8:41 a.m.
New subject: [libvirt] [PATCH 7/9] remote: Avoid coverity[leaked_storage] message.
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
10:07 a.m.
New subject: [libvirt] [PATCH 7/9] remote: Avoid coverity[leaked_storage] message.
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.
8:41 a.m.
New subject: [libvirt] [PATCH 8/9] network: Add coverity[leaked_handle] to ignore error
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
8:41 a.m.
New subject: [libvirt] [PATCH 9/9] storage: Add coverity[dead_error_condition] to avoid error
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
10:07 a.m.
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
4318
days inactive
4325
days old
22 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Michal Privoznik