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

[Eric Blake]

On 01/22/2013 12:28 PM, John Ferlan wrote:

>>> @@ -1795,8 +1795,11 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu,
>>>              return -1;
>>>  
>>>          memset(pm, 0, sizeof(cpumap_t));
>>> -        for (j = 0; j < maplen; j++)
>>> +        for (j = 0; j < maplen; j++) {
>>> +            /* coverity[ptr_arith] */
>>> +            /* coverity[sign_extension] */
>>>              *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7));
>>> +        }
>>
>> Having to add two comments to shut up Coverity feels awkward.  Would it
>> also work to do 'uint64_t j' instead of the current 'int j' in the
>> declaration a few lines earlier?  Not only would it be a smaller diff,
>> but the fewer Coverity comments we have to use, the better I feel.
>>
>> I know this has already been pushed, but it is still worth seeing if a
>> followup patch can clean things further.

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.

>>
> 
> Nope, just tried using uint64_t on 'j' without any luck.  I also tried putting the comments on the same line without the desired effect. Here's data on the two reported defects (I turned OFF line wrap for this - the line numbers are from an older analysis):
> 
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def1]
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1751: cond_false: Condition "hv_versions.hypervisor > 1", taking false branch
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1790: else_branch: Reached else branch
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: address_of: Taking address with "&xen_cpumap" yields a singleton pointer.
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1792: assign: Assigning: "pm" = "&xen_cpumap".
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "maplen > 8 /* (int)sizeof (cpumap_t) */", taking false branch
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1795: cond_false: Condition "0UL /* sizeof (cpumap_t) & 7 */", taking false branch
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1799: cond_true: Condition "j < maplen", taking true branch
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: ptr_arith: Using "pm" as an array.  This might corrupt or misinterpret adjacent memory locations.

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

> 
> AND
> 
> Error: SIGN_EXTENSION (CWE-194): [#def245]
> libvirt-1.0.0/src/xen/xen_hypervisor.c:1800: sign_extension: Suspicious implicit sign extension: "cpumap[j]" with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8 * (j & 7)" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned).  If "cpumap[j] << 8 * (j & 7)" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130128/cdef00e5/attachment-0001.sig>