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

On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote: ... 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