diff mbox series

s390: Fix undefined behaviour in feenableexcept, fedisableexcept [BZ #30960]

Message ID 20231018130840.3249206-1-stli@linux.ibm.com
State Committed
Headers
Series s390: Fix undefined behaviour in feenableexcept, fedisableexcept [BZ #30960] |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 fail Patch failed to apply

Commit Message

Stefan Liebler Oct. 18, 2023, 1:08 p.m. UTC 
  If feenableexcept or fedisableexcept gets excepts=FE_INVALID=0x80
as input, we have a signed left shift: 0x80 << 24 which is not
representable as int and thus is undefined behaviour according to
C standard.

This patch casts excepts as unsigned int before shifting, which is
defined.

For me, the observed undefined behaviour is that the shift is done
with "unsigned"-instructions, which is exactly what we want.
Furthermore, I don't get any exception-flags.

After the fix, the code is using the same instruction sequence as
before.
---
 sysdeps/s390/fpu/fedisblxcpt.c | 3 ++-
 sysdeps/s390/fpu/feenablxcpt.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Andreas Schwab Oct. 19, 2023, 8:48 a.m. UTC | #1 
On Okt 18 2023, Stefan Liebler wrote:

> diff --git a/sysdeps/s390/fpu/fedisblxcpt.c b/sysdeps/s390/fpu/fedisblxcpt.c
> index 728f103f43..84b2c5e64a 100644
> --- a/sysdeps/s390/fpu/fedisblxcpt.c
> +++ b/sysdeps/s390/fpu/fedisblxcpt.c
> @@ -26,7 +26,8 @@ fedisableexcept (int excepts)
>  
>    _FPU_GETCW (temp);
>    old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
> -  new_flags = (temp & (~((excepts & FE_ALL_EXCEPT) << FPC_EXCEPTION_MASK_SHIFT)));
> +  new_flags = (temp & (~(((unsigned int) excepts & FE_ALL_EXCEPT)
> +			 << FPC_EXCEPTION_MASK_SHIFT)));

There is a redundant pair of parens around the ~ operator that makes the
whole expression more difficult to read.  Please consider removing it.

>    _FPU_SETCW (new_flags);
>  
>    return old_exc;
> diff --git a/sysdeps/s390/fpu/feenablxcpt.c b/sysdeps/s390/fpu/feenablxcpt.c
> index 0807e610a2..76d25316f4 100644
> --- a/sysdeps/s390/fpu/feenablxcpt.c
> +++ b/sysdeps/s390/fpu/feenablxcpt.c
> @@ -26,7 +26,8 @@ feenableexcept (int excepts)
>  
>    _FPU_GETCW (temp);
>    old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
> -  new_flags = (temp | ((excepts & FE_ALL_EXCEPT) <<  FPC_EXCEPTION_MASK_SHIFT));
> +  new_flags = (temp | ((unsigned int) (excepts & FE_ALL_EXCEPT)
> +		       << FPC_EXCEPTION_MASK_SHIFT));
>    _FPU_SETCW (new_flags);
>  
>    return old_exc;

Please place the cast consistently (either inside or outside the
(excepts & FE_ALL_EXCEPT) expression).  Ok with that change.
Stefan Liebler Oct. 19, 2023, 12:31 p.m. UTC | #2 
On 19.10.23 10:48, Andreas Schwab wrote:
> On Okt 18 2023, Stefan Liebler wrote:
> 
>> diff --git a/sysdeps/s390/fpu/fedisblxcpt.c b/sysdeps/s390/fpu/fedisblxcpt.c
>> index 728f103f43..84b2c5e64a 100644
>> --- a/sysdeps/s390/fpu/fedisblxcpt.c
>> +++ b/sysdeps/s390/fpu/fedisblxcpt.c
>> @@ -26,7 +26,8 @@ fedisableexcept (int excepts)
>>  
>>    _FPU_GETCW (temp);
>>    old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
>> -  new_flags = (temp & (~((excepts & FE_ALL_EXCEPT) << FPC_EXCEPTION_MASK_SHIFT)));
>> +  new_flags = (temp & (~(((unsigned int) excepts & FE_ALL_EXCEPT)
>> +			 << FPC_EXCEPTION_MASK_SHIFT)));
> 
> There is a redundant pair of parens around the ~ operator that makes the
> whole expression more difficult to read.  Please consider removing it.
> 
>>    _FPU_SETCW (new_flags);
>>  
>>    return old_exc;
>> diff --git a/sysdeps/s390/fpu/feenablxcpt.c b/sysdeps/s390/fpu/feenablxcpt.c
>> index 0807e610a2..76d25316f4 100644
>> --- a/sysdeps/s390/fpu/feenablxcpt.c
>> +++ b/sysdeps/s390/fpu/feenablxcpt.c
>> @@ -26,7 +26,8 @@ feenableexcept (int excepts)
>>  
>>    _FPU_GETCW (temp);
>>    old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
>> -  new_flags = (temp | ((excepts & FE_ALL_EXCEPT) <<  FPC_EXCEPTION_MASK_SHIFT));
>> +  new_flags = (temp | ((unsigned int) (excepts & FE_ALL_EXCEPT)
>> +		       << FPC_EXCEPTION_MASK_SHIFT));
>>    _FPU_SETCW (new_flags);
>>  
>>    return old_exc;
> 
> Please place the cast consistently (either inside or outside the
> (excepts & FE_ALL_EXCEPT) expression).  Ok with that change.
> 

Thanks for the review. I've just committed it with your two mentioned
changes.

Thanks,
Stefan
diff mbox series

Patch

diff --git a/sysdeps/s390/fpu/fedisblxcpt.c b/sysdeps/s390/fpu/fedisblxcpt.c
index 728f103f43..84b2c5e64a 100644
--- a/sysdeps/s390/fpu/fedisblxcpt.c
+++ b/sysdeps/s390/fpu/fedisblxcpt.c
@@ -26,7 +26,8 @@  fedisableexcept (int excepts)
 
   _FPU_GETCW (temp);
   old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
-  new_flags = (temp & (~((excepts & FE_ALL_EXCEPT) << FPC_EXCEPTION_MASK_SHIFT)));
+  new_flags = (temp & (~(((unsigned int) excepts & FE_ALL_EXCEPT)
+			 << FPC_EXCEPTION_MASK_SHIFT)));
   _FPU_SETCW (new_flags);
 
   return old_exc;
diff --git a/sysdeps/s390/fpu/feenablxcpt.c b/sysdeps/s390/fpu/feenablxcpt.c
index 0807e610a2..76d25316f4 100644
--- a/sysdeps/s390/fpu/feenablxcpt.c
+++ b/sysdeps/s390/fpu/feenablxcpt.c
@@ -26,7 +26,8 @@  feenableexcept (int excepts)
 
   _FPU_GETCW (temp);
   old_exc = (temp & FPC_EXCEPTION_MASK) >> FPC_EXCEPTION_MASK_SHIFT;
-  new_flags = (temp | ((excepts & FE_ALL_EXCEPT) <<  FPC_EXCEPTION_MASK_SHIFT));
+  new_flags = (temp | ((unsigned int) (excepts & FE_ALL_EXCEPT)
+		       << FPC_EXCEPTION_MASK_SHIFT));
   _FPU_SETCW (new_flags);
 
   return old_exc;