[libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

[John Ferlan]

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.
>