Patchwork Fix up undefined signed overflows in FIXED_SSNEG (PR libgcc/55451)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 11, 2012, 12:55 p.m.
Message ID <20121211125535.GO2315@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/205207/
State New
Headers show

Comments

Jakub Jelinek - Dec. 11, 2012, 12:55 p.m.
Hi!

This routine, besides aspiring to win obfuscated C contest (not trying to
address that) contains two undefined signed overflows, which presumably
show up in arm testing.  One overflow is on z = x - y; line,
where the x looks just like obfuscation (it is always 0), for input equal to
INT_MIN bits (from what I understand, the routine wants to return normal
negation of all values but INT_MIN, which is instead replaced with INT_MAX).
Fixed by doing the subtraction (== negation) in UINT_C_TYPE instead.
Another issue is that ((INT_C_TYPE) 1) << I_F_BITS, if it is equal to
INT_MIN, overflows on z-- (x >= 0 is always true, another obfuscation).

Untested, Greta, can you please test this on arm?  Ok for trunk?

2012-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR libgcc/55451
	* fixed-bit.c (FIXED_SSNEG): Avoid undefined signed overflows.


	Jakub
Ian Taylor - Dec. 11, 2012, 2:52 p.m.
On Tue, Dec 11, 2012 at 4:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> This routine, besides aspiring to win obfuscated C contest (not trying to
> address that) contains two undefined signed overflows, which presumably
> show up in arm testing.  One overflow is on z = x - y; line,
> where the x looks just like obfuscation (it is always 0), for input equal to
> INT_MIN bits (from what I understand, the routine wants to return normal
> negation of all values but INT_MIN, which is instead replaced with INT_MAX).
> Fixed by doing the subtraction (== negation) in UINT_C_TYPE instead.
> Another issue is that ((INT_C_TYPE) 1) << I_F_BITS, if it is equal to
> INT_MIN, overflows on z-- (x >= 0 is always true, another obfuscation).
>
> Untested, Greta, can you please test this on arm?  Ok for trunk?
>
> 2012-12-11  Jakub Jelinek  <jakub@redhat.com>
>
>         PR libgcc/55451
>         * fixed-bit.c (FIXED_SSNEG): Avoid undefined signed overflows.
>
> --- gcc/fixed-bit.c.jj  2011-11-04 07:49:37.000000000 +0100
> +++ gcc/fixed-bit.c     2012-12-11 13:16:53.701767571 +0100
> @@ -1,5 +1,5 @@
>  /* This is a software fixed-point library.
> -   Copyright (C) 2007, 2009, 2011 Free Software Foundation, Inc.
> +   Copyright (C) 2007, 2009, 2011, 2012 Free Software Foundation, Inc.
>
>  This file is part of GCC.
>
> @@ -569,16 +569,11 @@ FIXED_SSNEG (FIXED_C_TYPE a)
>    INT_C_TYPE x, y, z;
>    memcpy (&y, &a, FIXED_SIZE);
>    x = 0;
> -  z = x - y;
> +  z = x - (UINT_C_TYPE) y;
>    if (((x ^ y) >> I_F_BITS) & 1)
>      {
>        if (((z ^ x) >> I_F_BITS) & 1)
> -        {
> -          z = 1;
> -          z = z << I_F_BITS;
> -          if (x >= 0)
> -            z--;
> -        }
> +       z = (((UINT_C_TYPE) 1) << I_F_BITS) - 1;
>      }
>  #if HAVE_PADDING_BITS
>    z = z << PADDING_BITS;


I presume that the intent of the obfuscation is to make SSNEG look
exactly like SSSUB.  And SSSUB appears to have the exact problems that
you are fixing here.  So I think you ought to fix both the same way.

Ian

Patch

--- gcc/fixed-bit.c.jj	2011-11-04 07:49:37.000000000 +0100
+++ gcc/fixed-bit.c	2012-12-11 13:16:53.701767571 +0100
@@ -1,5 +1,5 @@ 
 /* This is a software fixed-point library.
-   Copyright (C) 2007, 2009, 2011 Free Software Foundation, Inc.
+   Copyright (C) 2007, 2009, 2011, 2012 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -569,16 +569,11 @@  FIXED_SSNEG (FIXED_C_TYPE a)
   INT_C_TYPE x, y, z;
   memcpy (&y, &a, FIXED_SIZE);
   x = 0;
-  z = x - y;
+  z = x - (UINT_C_TYPE) y;
   if (((x ^ y) >> I_F_BITS) & 1)
     {
       if (((z ^ x) >> I_F_BITS) & 1)
-        {
-          z = 1;
-          z = z << I_F_BITS;
-          if (x >= 0)
-            z--;
-        }
+	z = (((UINT_C_TYPE) 1) << I_F_BITS) - 1;
     }
 #if HAVE_PADDING_BITS
   z = z << PADDING_BITS;