Cornelia Huck <cornelia.h...@de.ibm.com> writes:
> On Tue, 20 Jan 2015 10:45:41 +0100
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> This patch makes Coverity unhappy:
>>
>> *** CID 1264326: Unintended sign extension (SIGN_EXTENSION)
>> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call()
>> 781 stq_p(&fib.pal, pbdev->pal);
>> 782 stq_p(&fib.iota, pbdev->g_iota);
>> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr);
>> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
>> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
>> 786
>> >>> CID 1264326: Unintended sign extension (SIGN_EXTENSION)
>> >>> Suspicious implicit sign extension: "pbdev->isc" with type
>> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc <<
>> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then
>> >>> sign-extended to type "unsigned long" (64 bits, unsigned). If
>> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than
>> >>> 0x7FFFFFFF, the upper bits of the result will all be 1.
>> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
>> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
>> 789 pbdev->routes.adapter.summary_offset;
>> 790 stw_p(&fib.data, data);
>> 791
>> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>
> There's a fix for this (and the memory leak):
>
> http://marc.info/?l=qemu-devel&m=142124886620078&w=2
>
> The patch is sitting in my queue, will send with the next pile of s390x
> updates.
I can't see how
@@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t
fiba)
data = (pbdev->isc << 28) | (pbdev->noi << 16) |
(pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
pbdev->routes.adapter.summary_offset;
- stw_p(&fib.data, data);
+ stl_p(&fib.data, data);
if (pbdev->fh >> ENABLE_BIT_OFFSET) {
fib.fc |= 0x80;
fixes the implicit sign extension within the assignment preceding it.
Let me explain it again real slow:
1. pbdev->isc gets promoted from uint8_t to int as operand of binary <<
(usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8)
2. The int result is shifted left 28 bits. This can set the MSB.
3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted
left 16 bits.
4. The two shift results stay int and get ored.
5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left
8 bits.
6. The next or's left operand is the int result of 4 and the right
operant is the uint64_t result of 5. Therefore, the left operand is
*sign-extended* from int to uint64_t. This copies bit#7 of
pbdev->isc to bits#31..63. Whoops.
Regarding the leak, I prefer my patch, because it avoids the free on
error. But you're the maintainer.
