On 01/28/2013 06:52 PM, Eric Blake wrote:
> 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));
s/*xen_cpumap/xen_cpumap
-> Compiler error
> 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.
>
We get a SIGN_EXTENSION bug:
(1) Event sign_extension: Suspicious implicit sign extension: "cpumap[j]"
with type "unsigned char" (8 bits, unsigned) is promoted in "cpumap[j] << 8ULL
* (j & 7ULL)" to type "int" (32 bits, signed), then sign-extended to type
"unsigned long" (64 bits, unsigned). If "cpumap[j] << 8ULL * (j & 7ULL)" is
greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
1815 val |= cpumap[j] << (8ULL * (j & 7ULL));
I initially tried changing "j" to uint64_t and 7 to 7ULL to no avail... When I
read the text, googled, which indicated that the unsigned char array cpumap
will be promoted to an 'int' when using arithmetic shift, so I went the other
way and changed 'val' an 'int' and the 8ULL to just 8.
That removed the SIGN_EXTENSION leaving a DEADCODE error
1813 memset(&xen_cpumap, 0, sizeof(xen_cpumap));
(1) Event assignment: Assigning: "j" = "0".
(2) Event incr: Incrementing "j". The value of "j" is now 1.
(3) Event incr: Incrementing "j". The value of "j" is now 2.
Also see events: [at_least][dead_error_condition][dead_error_begin]
1814 for (j = 0; j < maplen; j++) {
1815 val |= cpumap[j] << (8 * (j & 7));
(4) Event at_least: At condition "j % 7 == 7", the value of "j" must be at
least 0.
(5) Event dead_error_condition: The condition "j % 7 == 7" cannot be
true.
Also see events: [assignment][incr][incr][dead_error_begin]
1816 if (j % 7 == 7) {
(6) Event dead_error_begin: Execution cannot reach this statement
"memcpy((char *)&xen_cpumap ...".
Also see events: [assignment][incr][incr][at_least][dead_error_condition]
1817 memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val));
1818 val = 0;
1819 }
1820 }
Changing the "j % 7 == 7" to "(j & 7) == 7" removed that condition (where the
parens around j&7 were required by the compiler.
Leaving the following which had no Coverity errors:
cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */
int 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] << (8 * (j & 7));
if ((j & 7) == 7) {
memcpy(((char *)&xen_cpumap) + j, &val, sizeof(val));
val = 0;
}
}
John
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
