[Qemu-devel] [PATCH] hw/ide/ahci.c: Fix shift left into sign bit

[Qemu-devel] [PATCH] hw/ide/ahci.c: Fix shift left into sign bit

[Peter Maydell]

Avoid undefined behaviour from shifting left into the sign bit:

hw/ide/ahci.c:551:36: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'

(Unfortunately C's promotion rules mean that in the expression
"some_uint8_t_variable << 24" the LHS gets promoted to signed
int before shifting.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
clang's undefined sanitizer produces a lot of copies of this warning during
'make check'...

 hw/ide/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 796be15..21f76ed 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -548,7 +548,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
         ad->init_d2h_sent = true;
         /* We're emulating receiving the first Reg H2D Fis from the device;
          * Update the SIG register, but otherwise proceed as normal. */
-        pr->sig = (ide_state->hcyl << 24) |
+        pr->sig = ((uint32_t)ide_state->hcyl << 24) |
             (ide_state->lcyl << 16) |
             (ide_state->sector << 8) |
             (ide_state->nsector & 0xFF);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] hw/ide/ahci.c: Fix shift left into sign bit

[John Snow]

On 10/16/2015 01:48 PM, Peter Maydell wrote:
> Avoid undefined behaviour from shifting left into the sign bit:
> 
> hw/ide/ahci.c:551:36: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
> 
> (Unfortunately C's promotion rules mean that in the expression
> "some_uint8_t_variable << 24" the LHS gets promoted to signed
> int before shifting.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> clang's undefined sanitizer produces a lot of copies of this warning during
> 'make check'...
> 
>  hw/ide/ahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 796be15..21f76ed 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -548,7 +548,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
>          ad->init_d2h_sent = true;
>          /* We're emulating receiving the first Reg H2D Fis from the device;
>           * Update the SIG register, but otherwise proceed as normal. */
> -        pr->sig = (ide_state->hcyl << 24) |
> +        pr->sig = ((uint32_t)ide_state->hcyl << 24) |
>              (ide_state->lcyl << 16) |
>              (ide_state->sector << 8) |
>              (ide_state->nsector & 0xFF);
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Since the "patches" tool seems to still be hiccuping, do you want to
just apply this directly?

--js

Re: [Qemu-devel] [PATCH] hw/ide/ahci.c: Fix shift left into sign bit

[Peter Maydell]

On 16 October 2015 at 21:43, John Snow <jsnow@redhat.com> wrote:
>
>
> On 10/16/2015 01:48 PM, Peter Maydell wrote:
>> Avoid undefined behaviour from shifting left into the sign bit:
>>
>> hw/ide/ahci.c:551:36: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
>>
>> (Unfortunately C's promotion rules mean that in the expression
>> "some_uint8_t_variable << 24" the LHS gets promoted to signed
>> int before shifting.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> clang's undefined sanitizer produces a lot of copies of this warning during
>> 'make check'...
>>
>>  hw/ide/ahci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 796be15..21f76ed 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -548,7 +548,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
>>          ad->init_d2h_sent = true;
>>          /* We're emulating receiving the first Reg H2D Fis from the device;
>>           * Update the SIG register, but otherwise proceed as normal. */
>> -        pr->sig = (ide_state->hcyl << 24) |
>> +        pr->sig = ((uint32_t)ide_state->hcyl << 24) |
>>              (ide_state->lcyl << 16) |
>>              (ide_state->sector << 8) |
>>              (ide_state->nsector & 0xFF);
>>
>
> Reviewed-by: John Snow <jsnow@redhat.com>
>
> Since the "patches" tool seems to still be hiccuping, do you want to
> just apply this directly?

OK, applied to master.

thanks
-- PMM