7:15 a.m.
Refactor the code flow a bit more to clear coverity errors.
Store the cpu count in an intermediate variable and reuse it rather than
caluclating the index.
---
src/util/virprocess.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index a38cb75..c07e5cd 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
size_t i;
cpu_set_t *mask;
size_t masklen;
+ size_t ncpus;
virBitmapPtr ret = NULL;
# ifdef CPU_ALLOC
/* 262144 cpus ought to be enough for anyone */
- masklen = CPU_ALLOC_SIZE(1024 << 8);
- mask = CPU_ALLOC(1024 << 8);
+ ncpus = 1024 << 8;
+ masklen = CPU_ALLOC_SIZE(ncpus);
+ mask = CPU_ALLOC(ncpus);
if (!mask) {
virReportOOMError();
@@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
CPU_ZERO_S(masklen, mask);
# else
+ ncpus = 1024;
if (VIR_ALLOC(mask) < 0)
return NULL;
@@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
goto cleanup;
}
- if (!(ret = virBitmapNew(masklen * 8)))
+ if (!(ret = virBitmapNew(ncpus)))
goto cleanup;
- for (i = 0; i < masklen * 8; i++) {
+ for (i = 0; i < ncpus; i++) {
# ifdef CPU_ALLOC
if (CPU_ISSET_S(i, masklen, mask))
ignore_value(virBitmapSetBit(ret, i));
--
2.4.1
7:34 a.m.
On 06/04/2015 08:15 AM, Peter Krempa wrote:
Refactor the code flow a bit more to clear coverity errors.
Store the cpu count in an intermediate variable and reuse it rather than
caluclating the index.
---
src/util/virprocess.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index a38cb75..c07e5cd 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
size_t i;
cpu_set_t *mask;
size_t masklen;
+ size_t ncpus;
virBitmapPtr ret = NULL;
# ifdef CPU_ALLOC
/* 262144 cpus ought to be enough for anyone */
- masklen = CPU_ALLOC_SIZE(1024 << 8);
- mask = CPU_ALLOC(1024 << 8);
+ ncpus = 1024 << 8;
+ masklen = CPU_ALLOC_SIZE(ncpus);
+ mask = CPU_ALLOC(ncpus);
if (!mask) {
virReportOOMError();
@@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
CPU_ZERO_S(masklen, mask);
# else
+ ncpus = 1024;
if (VIR_ALLOC(mask) < 0)
return NULL;
@@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
goto cleanup;
}
- if (!(ret = virBitmapNew(masklen * 8)))
+ if (!(ret = virBitmapNew(ncpus)))
goto cleanup;
- for (i = 0; i < masklen * 8; i++) {
+ for (i = 0; i < ncpus; i++) {
# ifdef CPU_ALLOC
if (CPU_ISSET_S(i, masklen, mask))
^^^ Coverity still complains here
No real change since previous...
John
ignore_value(virBitmapSetBit(ret, i));
10:13 a.m.
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
On 06/04/2015 08:15 AM, Peter Krempa wrote:
> Refactor the code flow a bit more to clear coverity errors.
>
> Store the cpu count in an intermediate variable and reuse it rather than
> caluclating the index.
> ---
> src/util/virprocess.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index a38cb75..c07e5cd 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
> size_t i;
> cpu_set_t *mask;
> size_t masklen;
> + size_t ncpus;
> virBitmapPtr ret = NULL;
>
> # ifdef CPU_ALLOC
> /* 262144 cpus ought to be enough for anyone */
> - masklen = CPU_ALLOC_SIZE(1024 << 8);
> - mask = CPU_ALLOC(1024 << 8);
> + ncpus = 1024 << 8;
> + masklen = CPU_ALLOC_SIZE(ncpus);
> + mask = CPU_ALLOC(ncpus);
>
> if (!mask) {
> virReportOOMError();
> @@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
>
> CPU_ZERO_S(masklen, mask);
> # else
> + ncpus = 1024;
> if (VIR_ALLOC(mask) < 0)
> return NULL;
>
> @@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
> goto cleanup;
> }
>
> - if (!(ret = virBitmapNew(masklen * 8)))
> + if (!(ret = virBitmapNew(ncpus)))
> goto cleanup;
>
> - for (i = 0; i < masklen * 8; i++) {
> + for (i = 0; i < ncpus; i++) {
> # ifdef CPU_ALLOC
> if (CPU_ISSET_S(i, masklen, mask))
^^^ Coverity still complains here
No real change since previous...
Would you mind sharing the error after applying this patch?
Peter
10:28 a.m.
On 06/04/2015 11:13 AM, Peter Krempa wrote:
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
>
>
> On 06/04/2015 08:15 AM, Peter Krempa wrote:
>> Refactor the code flow a bit more to clear coverity errors.
>>
>> Store the cpu count in an intermediate variable and reuse it rather than
>> caluclating the index.
>> ---
>> src/util/virprocess.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>> index a38cb75..c07e5cd 100644
>> --- a/src/util/virprocess.c
>> +++ b/src/util/virprocess.c
>> @@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
>> size_t i;
>> cpu_set_t *mask;
>> size_t masklen;
>> + size_t ncpus;
>> virBitmapPtr ret = NULL;
>>
>> # ifdef CPU_ALLOC
>> /* 262144 cpus ought to be enough for anyone */
>> - masklen = CPU_ALLOC_SIZE(1024 << 8);
>> - mask = CPU_ALLOC(1024 << 8);
>> + ncpus = 1024 << 8;
>> + masklen = CPU_ALLOC_SIZE(ncpus);
>> + mask = CPU_ALLOC(ncpus);
>>
>> if (!mask) {
>> virReportOOMError();
>> @@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
>>
>> CPU_ZERO_S(masklen, mask);
>> # else
>> + ncpus = 1024;
>> if (VIR_ALLOC(mask) < 0)
>> return NULL;
>>
>> @@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
>> goto cleanup;
>> }
>>
>> - if (!(ret = virBitmapNew(masklen * 8)))
>> + if (!(ret = virBitmapNew(ncpus)))
>> goto cleanup;
>>
>> - for (i = 0; i < masklen * 8; i++) {
>> + for (i = 0; i < ncpus; i++) {
>> # ifdef CPU_ALLOC
>> if (CPU_ISSET_S(i, masklen, mask))
>
> ^^^ Coverity still complains here
>
> No real change since previous...
Would you mind sharing the error after applying this patch?
Peter
Sure (it's cut-n-paste)
471 virBitmapPtr
472 virProcessGetAffinity(pid_t pid)
473 {
474 size_t i;
475 cpu_set_t *mask;
476 size_t masklen;
477 size_t ncpus;
478 virBitmapPtr ret = NULL;
479
480 # ifdef CPU_ALLOC
481 /* 262144 cpus ought to be enough for anyone */
(1) Event assignment: Assigning: "ncpus" = "262144UL".
Also see events: [cond_at_most][assignment][overrun-local]
482 ncpus = 1024 << 8;
483 masklen = CPU_ALLOC_SIZE(ncpus);
484 mask = CPU_ALLOC(ncpus);
485
(2) Event cond_false: Condition "!mask", taking false branch
486 if (!mask) {
487 virReportOOMError();
488 return NULL;
(3) Event if_end: End of if statement
489 }
490
491 CPU_ZERO_S(masklen, mask);
492 # else
493 ncpus = 1024;
494 if (VIR_ALLOC(mask) < 0)
495 return NULL;
496
497 masklen = sizeof(*mask);
498 CPU_ZERO(mask);
499 # endif
500
(4) Event cond_false: Condition "sched_getaffinity(pid, masklen, mask) < 0",
taking false branch
501 if (sched_getaffinity(pid, masklen, mask) < 0) {
502 virReportSystemError(errno,
503 _("cannot get CPU affinity of process %d"),
pid);
504 goto cleanup;
(5) Event if_end: End of if statement
505 }
506
(6) Event cond_false: Condition "!(ret = virBitmapNew(ncpus))", taking false
branch
507 if (!(ret = virBitmapNew(ncpus)))
(7) Event if_end: End of if statement
508 goto cleanup;
509
(8) Event cond_true: Condition "i < ncpus", taking true branch
(13) Event loop_begin: Jumped back to beginning of loop
(14) Event cond_true: Condition "i < ncpus", taking true branch
(15) Event cond_at_most: Checking "i < ncpus" implies that "i" may
be up to 262143 on the true branch.
Also see events: [assignment][assignment][overrun-local]
510 for (i = 0; i < ncpus; i++) {
511 # ifdef CPU_ALLOC
(9) Event cond_true: Condition "__cpu / 8 < masklen", taking true branch
(10) Event cond_true: Condition "((__cpu_mask const *)mask->__bits[__cpu / (64UL
/* 8 * sizeof (__cpu_mask) */)] & (1UL /* (__cpu_mask)1 */ << __cpu % (64UL /* 8
* sizeof (__cpu_mask) */))) != 0", taking true branch
(11) Event cond_true: Condition "({...})", taking true branch
(16) Event assignment: Assigning: "__cpu" = "i". The value of
"__cpu" may now be up to 262143.
(17) Event cond_true: Condition "__cpu / 8 < masklen", taking true branch
(18) Event overrun-local: Overrunning array of 16 8-byte elements at element index 4095
(byte offset 32760) by dereferencing pointer "(__cpu_mask const *)mask->__bits +
__cpu / 64UL".
Also see events: [assignment][cond_at_most]
512 if (CPU_ISSET_S(i, masklen, mask))
513 ignore_value(virBitmapSetBit(ret, i));
514 # else
515 if (CPU_ISSET(i, mask))
516 ignore_value(virBitmapSetBit(ret, i));
517 # endif
(12) Event loop: Jumping back to the beginning of the loop
518 }
519
520 cleanup:
521 # ifdef CPU_ALLOC
522 CPU_FREE(mask);
523 # else
524 VIR_FREE(mask);
525 # endif
526
527 return ret;
528 }
8:41 a.m.
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
On 06/04/2015 08:15 AM, Peter Krempa wrote:
> Refactor the code flow a bit more to clear coverity errors.
>
> Store the cpu count in an intermediate variable and reuse it rather than
> caluclating the index.
> ---
> src/util/virprocess.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
...
^^^ Coverity still complains here
No real change since previous...
So I went in and traced it a bit. cpu_set_t is basically the following
structure: (assuming sizeof(unsigned long int) == 8))
typedef struct
{
unsigned long int __bits[1024 / 8 * 8];
} cpu_set_t;
The CPU_ALLOC() macro allocates an array of such structures. Since the
size of the __bits array is rounded nicely (128 bytes) the structure
itself does not add any padding and it's size is 128 bytes too the
structures are packed and thus the __bits fields of adjecent structures
basically form a longer array of unsigned long ints.
The CPU_ISSET_S macro then translates basically to this equivalent
function:
unsigned long int
CPU_ISSET_S(size_t __cpu,
size_t setsize,
cpu_set_t *cpusetp)
{
if (__cpu / 8 < setsize) {
unsigned long int subcpu = ((const unsigned long int *)(mask->__bits))[__cpu /
64];
unsigned long int mask = (unsigned long int) 1 << (__cpu % 64);
return subcpu & mask;
} else {
return 0;
}
}
The macro expects that the array is packed nicely and treats it
basically as a large array of unsigned ints. The macro then selects one
of the members and masks out the required cpu bit.
So then compiling the following proof of concept:
#define _GNU_SOURCE
#include <sched.h>
int main(void)
{
size_t ncpus = 1024 << 8;
size_t masklen = CPU_ALLOC_SIZE(ncpus);
cpu_set_t *mask = CPU_ALLOC(ncpus);
CPU_ZERO_S(masklen, mask);
for (size_t i = 0; i < ncpus; i++) {
if (CPU_ISSET_S(i, masklen, mask)) {
i = i;
}
}
CPU_FREE(mask);
return 0;
}
And running it in valgrind results in no error as expected:
$ valgrind ./a.out
==95609== Memcheck, a memory error detector
==95609== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==95609== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==95609== Command: ./a.out
==95609==
==95609==
==95609== HEAP SUMMARY:
==95609== in use at exit: 0 bytes in 0 blocks
==95609== total heap usage: 1 allocs, 1 frees, 32,768 bytes allocated
==95609==
==95609== All heap blocks were freed -- no leaks are possible
==95609==
==95609== For counts of detected and suppressed errors, rerun with: -v
==95609== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
The problem is that coverity thinks that the code overruns the __bits
array now that the code is simple enough to introspect which is
technically true. Previously the same situation would happen but the
loop that would resize the array incrementally probably was not
introspected properly and thus did not produce a warning.
So there are basically three options:
1) Silence the coverity warning
2) File a bug against libc or something to fix the macro
3) Reimplement CPU_ISSET_S internally. (Which I don't think will be
possible since cpu_set_t does not look public)
At any rate, there is no problem in libvirt's usage as it conforms to
the example in the man page.
As of this particular patch. It does not fix the coverity warning, but I
think the intention and code flow is more apparent once it's applied.
Peter
10:27 a.m.
On 06/10/2015 09:41 AM, Peter Krempa wrote:
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
> On 06/04/2015 08:15 AM, Peter Krempa wrote:
>> Refactor the code flow a bit more to clear coverity errors.
>>
>> Store the cpu count in an intermediate variable and reuse it rather than
>> caluclating the index.
>> ---
>> src/util/virprocess.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
...
>
> ^^^ Coverity still complains here
>
> No real change since previous...
>
nice detective work...
So I went in and traced it a bit. cpu_set_t is basically the following
structure: (assuming sizeof(unsigned long int) == 8))
typedef struct
{
unsigned long int __bits[1024 / 8 * 8];
} cpu_set_t;
The CPU_ALLOC() macro allocates an array of such structures. Since the
size of the __bits array is rounded nicely (128 bytes) the structure
itself does not add any padding and it's size is 128 bytes too the
structures are packed and thus the __bits fields of adjecent structures
basically form a longer array of unsigned long ints.
The CPU_ISSET_S macro then translates basically to this equivalent
function:
unsigned long int
CPU_ISSET_S(size_t __cpu,
size_t setsize,
cpu_set_t *cpusetp)
{
if (__cpu / 8 < setsize) {
unsigned long int subcpu = ((const unsigned long int *)(mask->__bits))[__cpu
/ 64];
unsigned long int mask = (unsigned long int) 1 << (__cpu % 64);
return subcpu & mask;
} else {
return 0;
}
}
The macro expects that the array is packed nicely and treats it
basically as a large array of unsigned ints. The macro then selects one
of the members and masks out the required cpu bit.
So then compiling the following proof of concept:
#define _GNU_SOURCE
#include <sched.h>
int main(void)
{
size_t ncpus = 1024 << 8;
size_t masklen = CPU_ALLOC_SIZE(ncpus);
cpu_set_t *mask = CPU_ALLOC(ncpus);
CPU_ZERO_S(masklen, mask);
for (size_t i = 0; i < ncpus; i++) {
if (CPU_ISSET_S(i, masklen, mask)) {
i = i;
}
}
CPU_FREE(mask);
return 0;
}
And running it in valgrind results in no error as expected:
$ valgrind ./a.out
==95609== Memcheck, a memory error detector
==95609== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==95609== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==95609== Command: ./a.out
==95609==
==95609==
==95609== HEAP SUMMARY:
==95609== in use at exit: 0 bytes in 0 blocks
==95609== total heap usage: 1 allocs, 1 frees, 32,768 bytes allocated
==95609==
==95609== All heap blocks were freed -- no leaks are possible
==95609==
==95609== For counts of detected and suppressed errors, rerun with: -v
==95609== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
The problem is that coverity thinks that the code overruns the __bits
array now that the code is simple enough to introspect which is
technically true. Previously the same situation would happen but the
loop that would resize the array incrementally probably was not
introspected properly and thus did not produce a warning.
So there are basically three options:
1) Silence the coverity warning
So on line just prior to CPU_ISSET_S either:
sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));
or
/* coverity[overrun-local] */
Silences the warning
ACK for the current adjustment - your call if you want the sa_assert or
the coverity noise.
John
2) File a bug against libc or something to fix the macro
3) Reimplement CPU_ISSET_S internally. (Which I don't think will be
possible since cpu_set_t does not look public)
At any rate, there is no problem in libvirt's usage as it conforms to
the example in the man page.
As of this particular patch. It does not fix the coverity warning, but I
think the intention and code flow is more apparent once it's applied.
4:18 p.m.
On 06/10/2015 09:27 AM, John Ferlan wrote:
> So there are basically three options:
> 1) Silence the coverity warning
So on line just prior to CPU_ISSET_S either:
sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));
Is that true even on 32-bit platforms?
or
/* coverity[overrun-local] */
Might be safer, even though I generally prefer sa_assert() over /* magic
comment */
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:49 a.m.
On Wed, Jun 10, 2015 at 15:18:15 -0600, Eric Blake wrote:
On 06/10/2015 09:27 AM, John Ferlan wrote:
>> So there are basically three options:
>> 1) Silence the coverity warning
>
> So on line just prior to CPU_ISSET_S either:
>
> sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));
Is that true even on 32-bit platforms?
This is never true. cpu_set_t is a structure that contains array of
unsigned longs that is 1024bits wide.
>
> or
>
> /* coverity[overrun-local] */
Might be safer, even though I generally prefer sa_assert() over /* magic
comment */
I'd go with this one in this case since the sa_assert one above probably
silences the warning in a bogous way.
3444
days inactive
3451
days old
7 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa