Re: [libvirt] [PATCH 4/9] xen: Add coverity[ptr_arith] and [sign_extension] tags

On 01/22/2013 12:28 PM, John Ferlan wrote: Ouch, we really DO have a bug, not to mention some very horrible code trying to do nasty aliasing that is not very portable. I'm surprised we don't have alignment complaints, by trying to treat cpumap_t as an array of 64-bit integers. This one, I don't know if we can silence without a coverity comment. Basically, it boils down to whether cpumap_t is typedef'd to something that can possibly be larger than 64 bits (it isn't - Coverity just confirmed that sizeof(cpumap_t) is 8 bytes). Since we just ensured that maplen will not go beyond the bounds of a 64-bit int array that overlays the same memory space, I'm okay with the /* coverity[ptr_arith] */ comment, but see below... Here is the real bug (but I'm surprised why it didn't go away when you changed j from int to int64_t). When j==4, you are attempting to do 'int << (8*4)'; but you _can't_ portably shift a 32-bit integer by any more than 31 bits. We _have_ to add in a type conversion to force this shift to occur in 64-bit math, such as: *(pm + (j / 8)) |= cpumap[j] << (8ULL * (j & 7)); Or better yet, why even futz around with 64-bit aliasing? It looks like this code is trying to take endian-independent input and force it into an endian-dependent xen_cpumap variable. I think it might be cleaner as: } else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ uint64_t val = 0; int j; if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) return -1; memset(&xen_cpumap, 0, sizeof(*xen_cpumap)); for (j = 0; j < maplen; j++) { val |= cpumap[j] << (8ULL * (j & 7)); if (j % 7 == 7) { memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val)); val = 0; } } and see if that shuts up Coverity. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org