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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list