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

[John Ferlan]

On 01/22/2013 11:35 AM, Eric Blake wrote:
> On 01/22/2013 07:40 AM, John Ferlan wrote:
>> The old cpu bitmap setting algorithm causes a couple of complaints which
>> have been tagged.
>> ---
>>  src/xen/xen_hypervisor.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
>> index a770f53..bfee56d 100644
>> --- a/src/xen/xen_hypervisor.c
>> +++ b/src/xen/xen_hypervisor.c
>> @@ -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.
> 

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.

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.



John