Patchwork Don't always instrument shifts (PR sanitizer/58413)

login
register
mail settings
Submitter Marek Polacek
Date Sept. 17, 2013, 10:32 a.m.
Message ID <20130917103203.GG23899@redhat.com>
Download mbox | patch
Permalink /patch/275418/
State New
Headers show

Comments

Marek Polacek - Sept. 17, 2013, 10:32 a.m.
On Mon, Sep 16, 2013 at 08:35:35PM +0200, Jakub Jelinek wrote:
> On Fri, Sep 13, 2013 at 08:01:36PM +0200, Marek Polacek wrote:
> I'd say the above is going to be a maintainance nightmare, with all the code
> duplication.  And you are certainly going to miss cases that way,
> e.g.
> void
> foo (void)
> {
>   int A[-2 / -1] = {};
> }
> 
> I'd say instead of adding all this, you should just at the right spot insert
> if (integer_zerop (t)) return NULL_TREE; or similar.
> 
> For shift instrumentation, I guess you could add
> if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
>   return NULL_TREE;
> right before:
>   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);

Yeah, this is _much_ better.  I'm glad we can live without that
ugliness.

> > +/* PR sanitizer/58413 */
> > +/* { dg-do run } */
> > +/* { dg-options "-fsanitize=shift -w" } */
> > +
> > +int x = 7;
> > +int
> > +main (void)
> > +{
> > +  /* All of the following should pass.  */
> > +  int A[128 >> 5] = {};
> > +  int B[128 << 5] = {};
> > +
> > +  static int e =
> > +    ((int)
> > +     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> > +      ((0) << ((15) + 6)) | ((0) << (15))));
> 
> This relies on int32plus, so needs to be /* { dg-do run { target int32plus } } */

Fixed.

> > --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3	2013-09-13 18:25:06.195847144 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-13 19:16:38.990211229 +0200
> > @@ -0,0 +1,21 @@
> > +/* { dg-do compile} */
> > +/* { dg-options "-fsanitize=shift -w" } */
> > +/* { dg-shouldfail "ubsan" } */
> > +
> > +int x;
> > +int
> > +main (void)
> > +{
> > +  /* None of the following should pass.  */
> > +  switch (x)
> > +    {
> > +    case 1 >> -1:	/* { dg-error "" } */
> > +    case -1 >> -1:	/* { dg-error "" } */
> > +    case 1 << -1:	/* { dg-error "" } */
> > +    case -1 << -1:	/* { dg-error "" } */
> > +    case -1 >> 200:	/* { dg-error "" } */
> > +    case 1 << 200:	/* { dg-error "" } */
> 
> Can't you fill in the error you are expecting, or is the problem
> that the wording is very different between C and C++?

I discovered { target c } stuff, so I filled in both error messages.

This patch seems to work: bootstrap-ubsan passes + ubsan testsuite
passes too.  Ok for trunk?

2013-09-17  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/58413
c-family/
	* c-ubsan.c (ubsan_instrument_shift): Don't instrument
	an expression if we can prove it is correct.
	(ubsan_instrument_division): Likewise.  Remove unnecessary
	check.

testsuite/
	* c-c++-common/ubsan/shift-4.c: New test.
	* c-c++-common/ubsan/shift-5.c: New test.
	* c-c++-common/ubsan/div-by-zero-5.c: New test.
	* gcc.dg/ubsan/c-shift-1.c: New test.


	Marek
Marek Polacek - Sept. 25, 2013, 8:34 a.m.
Ping.

On Tue, Sep 17, 2013 at 12:32:03PM +0200, Marek Polacek wrote:
> On Mon, Sep 16, 2013 at 08:35:35PM +0200, Jakub Jelinek wrote:
> > On Fri, Sep 13, 2013 at 08:01:36PM +0200, Marek Polacek wrote:
> > I'd say the above is going to be a maintainance nightmare, with all the code
> > duplication.  And you are certainly going to miss cases that way,
> > e.g.
> > void
> > foo (void)
> > {
> >   int A[-2 / -1] = {};
> > }
> > 
> > I'd say instead of adding all this, you should just at the right spot insert
> > if (integer_zerop (t)) return NULL_TREE; or similar.
> > 
> > For shift instrumentation, I guess you could add
> > if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
> >   return NULL_TREE;
> > right before:
> >   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
> 
> Yeah, this is _much_ better.  I'm glad we can live without that
> ugliness.
> 
> > > +/* PR sanitizer/58413 */
> > > +/* { dg-do run } */
> > > +/* { dg-options "-fsanitize=shift -w" } */
> > > +
> > > +int x = 7;
> > > +int
> > > +main (void)
> > > +{
> > > +  /* All of the following should pass.  */
> > > +  int A[128 >> 5] = {};
> > > +  int B[128 << 5] = {};
> > > +
> > > +  static int e =
> > > +    ((int)
> > > +     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> > > +      ((0) << ((15) + 6)) | ((0) << (15))));
> > 
> > This relies on int32plus, so needs to be /* { dg-do run { target int32plus } } */
> 
> Fixed.
> 
> > > --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3	2013-09-13 18:25:06.195847144 +0200
> > > +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-13 19:16:38.990211229 +0200
> > > @@ -0,0 +1,21 @@
> > > +/* { dg-do compile} */
> > > +/* { dg-options "-fsanitize=shift -w" } */
> > > +/* { dg-shouldfail "ubsan" } */
> > > +
> > > +int x;
> > > +int
> > > +main (void)
> > > +{
> > > +  /* None of the following should pass.  */
> > > +  switch (x)
> > > +    {
> > > +    case 1 >> -1:	/* { dg-error "" } */
> > > +    case -1 >> -1:	/* { dg-error "" } */
> > > +    case 1 << -1:	/* { dg-error "" } */
> > > +    case -1 << -1:	/* { dg-error "" } */
> > > +    case -1 >> 200:	/* { dg-error "" } */
> > > +    case 1 << 200:	/* { dg-error "" } */
> > 
> > Can't you fill in the error you are expecting, or is the problem
> > that the wording is very different between C and C++?
> 
> I discovered { target c } stuff, so I filled in both error messages.
> 
> This patch seems to work: bootstrap-ubsan passes + ubsan testsuite
> passes too.  Ok for trunk?
> 
> 2013-09-17  Marek Polacek  <polacek@redhat.com>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/58413
> c-family/
> 	* c-ubsan.c (ubsan_instrument_shift): Don't instrument
> 	an expression if we can prove it is correct.
> 	(ubsan_instrument_division): Likewise.  Remove unnecessary
> 	check.
> 
> testsuite/
> 	* c-c++-common/ubsan/shift-4.c: New test.
> 	* c-c++-common/ubsan/shift-5.c: New test.
> 	* c-c++-common/ubsan/div-by-zero-5.c: New test.
> 	* gcc.dg/ubsan/c-shift-1.c: New test.
> 
> --- gcc/c-family/c-ubsan.c.mp	2013-09-17 12:24:44.582835840 +0200
> +++ gcc/c-family/c-ubsan.c	2013-09-17 12:24:48.772849823 +0200
> @@ -51,14 +51,6 @@ ubsan_instrument_division (location_t lo
>    if (TREE_CODE (type) != INTEGER_TYPE)
>      return NULL_TREE;
>  
> -  /* If we *know* that the divisor is not -1 or 0, we don't have to
> -     instrument this expression.
> -     ??? We could use decl_constant_value to cover up more cases.  */
> -  if (TREE_CODE (op1) == INTEGER_CST
> -      && integer_nonzerop (op1)
> -      && !integer_minus_onep (op1))
> -    return NULL_TREE;
> -
>    t = fold_build2 (EQ_EXPR, boolean_type_node,
>  		    op1, build_int_cst (type, 0));
>  
> @@ -74,6 +66,11 @@ ubsan_instrument_division (location_t lo
>        t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
>      }
>  
> +  /* If the condition was folded to 0, no need to instrument
> +     this expression.  */
> +  if (integer_zerop (t))
> +    return NULL_TREE;
> +
>    /* In case we have a SAVE_EXPR in a conditional context, we need to
>       make sure it gets evaluated before the condition.  */
>    t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
> @@ -138,6 +135,11 @@ ubsan_instrument_shift (location_t loc,
>        tt = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, x, tt);
>      }
>  
> +  /* If the condition was folded to 0, no need to instrument
> +     this expression.  */
> +  if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
> +    return NULL_TREE;
> +
>    /* In case we have a SAVE_EXPR in a conditional context, we need to
>       make sure it gets evaluated before the condition.  */
>    t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
> --- gcc/testsuite/c-c++-common/ubsan/shift-4.c.mp	2013-09-17 12:25:12.130931875 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/shift-4.c	2013-09-17 10:19:44.665199565 +0200
> @@ -0,0 +1,30 @@
> +/* PR sanitizer/58413 */
> +/* { dg-do run { target int32plus } } */
> +/* { dg-options "-fsanitize=shift -w" } */
> +
> +int x = 7;
> +int
> +main (void)
> +{
> +  /* All of the following should pass.  */
> +  int A[128 >> 5] = {};
> +  int B[128 << 5] = {};
> +
> +  static int e =
> +    ((int)
> +     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> +      ((0) << ((15) + 6)) | ((0) << (15))));
> +
> +  if (e != 503316480)
> +    __builtin_abort ();
> +
> +  switch (x)
> +    {
> +    case 1 >> 4:
> +    case 1 << 4:
> +    case 128 << (4 + 1):
> +    case 128 >> (4 + 1):
> +      return 1;
> +    }
> +  return 0;
> +}
> --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp	2013-09-17 12:25:17.549952906 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-17 12:22:47.658413113 +0200
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize=shift -w" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int x;
> +int
> +foo (void)
> +{
> +  /* None of the following should pass.  */
> +  switch (x)
> +    {
> +    case 1 >> -1:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 12 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 12 } */
> +    case -1 >> -1:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 15 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 15 } */
> +    case 1 << -1:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 18 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 18 } */
> +    case -1 << -1:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 21 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 21 } */
> +    case -1 >> 200:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 24 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 24 } */
> +    case 1 << 200:
> +/* { dg-error "case label does not reduce to an integer constant" "" {target c } 27 } */
> +/* { dg-error "is not a constant expression" "" { target c++ } 27 } */
> +      return 1;
> +    }
> +  return 0;
> +}
> --- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c.mp	2013-09-17 12:25:05.910907983 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c	2013-09-17 10:19:44.724199779 +0200
> @@ -0,0 +1,8 @@
> +/* { dg-do compile} */
> +/* { dg-options "-fsanitize=integer-divide-by-zero" } */
> +
> +void
> +foo (void)
> +{
> +  int A[-2 / -1] = {};
> +}
> --- gcc/testsuite/gcc.dg/ubsan/c-shift-1.c.mp	2013-09-17 12:25:23.533975060 +0200
> +++ gcc/testsuite/gcc.dg/ubsan/c-shift-1.c	2013-09-17 10:19:44.725199783 +0200
> @@ -0,0 +1,18 @@
> +/* { dg-do compile} */
> +/* { dg-options "-fsanitize=shift -w" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int x;
> +int
> +main (void)
> +{
> +  /* None of the following should pass.  */
> +  int A[1 >> -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
> +  int B[-1 >> -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
> +  int D[1 << -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
> +  int E[-1 << -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
> +  int F[-1 >> 200] = {};  /* { dg-error "variable-sized object may not be initialized" } */
> +  int G[1 << 200] = {};   /* { dg-error "variable-sized object may not be initialized" } */
> +
> +  return 0;
> +}
> 
> 	Marek

	Marek
Jakub Jelinek - Sept. 25, 2013, 8:39 a.m.
On Wed, Sep 25, 2013 at 10:34:42AM +0200, Marek Polacek wrote:
> > 2013-09-17  Marek Polacek  <polacek@redhat.com>
> > 	    Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR sanitizer/58413
> > c-family/
> > 	* c-ubsan.c (ubsan_instrument_shift): Don't instrument
> > 	an expression if we can prove it is correct.
> > 	(ubsan_instrument_division): Likewise.  Remove unnecessary
> > 	check.
> > 
> > testsuite/
> > 	* c-c++-common/ubsan/shift-4.c: New test.
> > 	* c-c++-common/ubsan/shift-5.c: New test.
> > 	* c-c++-common/ubsan/div-by-zero-5.c: New test.
> > 	* gcc.dg/ubsan/c-shift-1.c: New test.

Ok.

	Jakub

Patch

--- gcc/c-family/c-ubsan.c.mp	2013-09-17 12:24:44.582835840 +0200
+++ gcc/c-family/c-ubsan.c	2013-09-17 12:24:48.772849823 +0200
@@ -51,14 +51,6 @@  ubsan_instrument_division (location_t lo
   if (TREE_CODE (type) != INTEGER_TYPE)
     return NULL_TREE;
 
-  /* If we *know* that the divisor is not -1 or 0, we don't have to
-     instrument this expression.
-     ??? We could use decl_constant_value to cover up more cases.  */
-  if (TREE_CODE (op1) == INTEGER_CST
-      && integer_nonzerop (op1)
-      && !integer_minus_onep (op1))
-    return NULL_TREE;
-
   t = fold_build2 (EQ_EXPR, boolean_type_node,
 		    op1, build_int_cst (type, 0));
 
@@ -74,6 +66,11 @@  ubsan_instrument_division (location_t lo
       t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
     }
 
+  /* If the condition was folded to 0, no need to instrument
+     this expression.  */
+  if (integer_zerop (t))
+    return NULL_TREE;
+
   /* In case we have a SAVE_EXPR in a conditional context, we need to
      make sure it gets evaluated before the condition.  */
   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
@@ -138,6 +135,11 @@  ubsan_instrument_shift (location_t loc,
       tt = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, x, tt);
     }
 
+  /* If the condition was folded to 0, no need to instrument
+     this expression.  */
+  if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
+    return NULL_TREE;
+
   /* In case we have a SAVE_EXPR in a conditional context, we need to
      make sure it gets evaluated before the condition.  */
   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
--- gcc/testsuite/c-c++-common/ubsan/shift-4.c.mp	2013-09-17 12:25:12.130931875 +0200
+++ gcc/testsuite/c-c++-common/ubsan/shift-4.c	2013-09-17 10:19:44.665199565 +0200
@@ -0,0 +1,30 @@ 
+/* PR sanitizer/58413 */
+/* { dg-do run { target int32plus } } */
+/* { dg-options "-fsanitize=shift -w" } */
+
+int x = 7;
+int
+main (void)
+{
+  /* All of the following should pass.  */
+  int A[128 >> 5] = {};
+  int B[128 << 5] = {};
+
+  static int e =
+    ((int)
+     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
+      ((0) << ((15) + 6)) | ((0) << (15))));
+
+  if (e != 503316480)
+    __builtin_abort ();
+
+  switch (x)
+    {
+    case 1 >> 4:
+    case 1 << 4:
+    case 128 << (4 + 1):
+    case 128 >> (4 + 1):
+      return 1;
+    }
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp	2013-09-17 12:25:17.549952906 +0200
+++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-17 12:22:47.658413113 +0200
@@ -0,0 +1,33 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -w" } */
+/* { dg-shouldfail "ubsan" } */
+
+int x;
+int
+foo (void)
+{
+  /* None of the following should pass.  */
+  switch (x)
+    {
+    case 1 >> -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 12 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 12 } */
+    case -1 >> -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 15 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 15 } */
+    case 1 << -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 18 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 18 } */
+    case -1 << -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 21 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 21 } */
+    case -1 >> 200:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 24 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 24 } */
+    case 1 << 200:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 27 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 27 } */
+      return 1;
+    }
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c.mp	2013-09-17 12:25:05.910907983 +0200
+++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c	2013-09-17 10:19:44.724199779 +0200
@@ -0,0 +1,8 @@ 
+/* { dg-do compile} */
+/* { dg-options "-fsanitize=integer-divide-by-zero" } */
+
+void
+foo (void)
+{
+  int A[-2 / -1] = {};
+}
--- gcc/testsuite/gcc.dg/ubsan/c-shift-1.c.mp	2013-09-17 12:25:23.533975060 +0200
+++ gcc/testsuite/gcc.dg/ubsan/c-shift-1.c	2013-09-17 10:19:44.725199783 +0200
@@ -0,0 +1,18 @@ 
+/* { dg-do compile} */
+/* { dg-options "-fsanitize=shift -w" } */
+/* { dg-shouldfail "ubsan" } */
+
+int x;
+int
+main (void)
+{
+  /* None of the following should pass.  */
+  int A[1 >> -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
+  int B[-1 >> -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+  int D[1 << -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
+  int E[-1 << -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+  int F[-1 >> 200] = {};  /* { dg-error "variable-sized object may not be initialized" } */
+  int G[1 << 200] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+
+  return 0;
+}