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

[John Ferlan]

On 01/29/2013 01:12 PM, Eric Blake wrote:
> On 01/29/2013 06:41 AM, John Ferlan wrote:
> 
>>>>>>              *(pm + (j / 8)) |= cpumap[j] << (8 * (j & 7));
> 
>> FWIW:  This path of code only occurs when "(hv_versions.hypervisor <=
>> 1)" - IOW really old code.  I checked history and the code was added by
>> commit id '86247f2c'.  Also since the target of the "|" operation is a
>> 'uint64_t' (e.g. *(pm + (j / 8)), wouldn't the shift from 0->56 be OK
>> (e.g. (8 * (j & 7)))?  That is it's not an 'int << (8*4)' it's a
>> 'uint64_t << (8*4)'.
> 
> Order of operations, broken down by type:
> 
> *(uint64_t* + (int / int)) |= unsigned char << (int * (int & int))
> *(uint64_t* + int) |= unsigned char << int
> *(uint64_t*) |= int << int
> uint64_t = uint64_t | int
> 
> There really is a sign extension bug, because 'unsigned char << int'
> uses 'int' math, but we then widen the int into uint64_t.  If vcpu 31 is
> turned on, we end up sign-extending and also enabling vcpus 32-63 at the
> same time, which was not the goal.
> 


The following silences the [sign_extension] issue:

    *(pm + (j / 8)) |= (uint64_t)cpumap[j] << (8 * (j & 7));

The following silences the [ptr_arith] issue:

    pm = (uint64_t *)((uint64_t)&xen_cpumap + (j & ~0x7UL));
    *pm |= (uint64_t)cpumap[j] << (8 * (j & 7));

Of course the code from my first response last night also silences both.


>>
>> When first approaching this I figured I didn't want to introduce a bug
>> into code that's been around a long time and that may not have any one
>> using it. I agree the line looks ugly and it did take me a bit to think
>> about it.
> 
> Just because no one has reported the bug or tested it lately doesn't
> mean that the bug isn't there - it has been there a LONG time. :)
> 

Understood. Nobody's complained either so no one's been able to fix
what's not broke :-)

>>
>> Mathematically what you propose with the memcpy() works; however, I come
>> from an architecture where a memcpy to an unaligned address causes a
>> fault
> 
> Such a memcpy() implementation would be flawed, and should be reported
> as a bug to that vendor; thankfully, these days, most libc do not have
> that flaw.  The C standard guarantees that memcpy() is required to work
> on unaligned copies (for a certain definition of work; you may still end
> up splicing together unrelated portions of adjacent integers when
> reading things back as integers, but byte-for-byte, the operation is
> well-defined).
> 

Alpha and Itanium have some (silent unless noise enabled) heartache when
the "to" address is "0x7ffe8f2b8cf1" (or x2, x3, x5, x6, x7, etc).
Ending with x0 & x8 are preferred while x4 & xc tolerated. If the noise
isn't turned on, the application performs poorly due to excessive
alignment faults.

John