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