diff mbox

Properly honor -fsanitize options (PR sanitizer/58443)

Message ID 20130918131042.GF15960@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 18, 2013, 1:10 p.m. UTC 
As mentioned in the PR, we weren't properly using the SANITIZE_* flags,
which resulted in sanitizing shifts even though we only should sanitize
divisions and vice versa.  It also removed one unnecessary check; it's
sufficient to check that instrument_expr != NULL.  I also found a mistake
in a test, it assumed that -fsanitize=shift also turns sanitizing of
divisions.  Thus fixed.

Ran ubsan testsuite/bootstrap-ubsan on x86_64-linux, ok for trunk?

2013-09-18  Marek Polacek  <polacek@redhat.com>

	PR sanitize/58443
cp/
	* typeck.c (cp_build_binary_op): Properly honor -fsanitize options.
	Remove unnecessary check.

c/
	* c-typeck.c (build_binary_op): Properly honor -fsanitize options.
	Remove unnecessary check.

testsuite/
	* g++.dg/ubsan/div-by-zero-1.C: Use the integer-divide-by-zero option
	instead of the shift option.
	* c-c++-common/ubsan/pr58443-1.c: New test.
	* c-c++-common/ubsan/pr58443-3.c: New test.
	* c-c++-common/ubsan/pr58443-2.c: New test.


	Marek

Comments

Jakub Jelinek Sept. 18, 2013, 1:15 p.m. UTC | #1 
On Wed, Sep 18, 2013 at 03:10:42PM +0200, Marek Polacek wrote:
> --- gcc/cp/typeck.c.mp	2013-09-18 14:00:14.303869196 +0200
> +++ gcc/cp/typeck.c	2013-09-18 14:08:21.287770112 +0200
> @@ -4884,7 +4884,7 @@ cp_build_binary_op (location_t location,
>    if (build_type == NULL_TREE)
>      build_type = result_type;
>  
> -  if ((flag_sanitize & SANITIZE_UNDEFINED)
> +  if ((flag_sanitize & SANITIZE_SHIFT || flag_sanitize & SANITIZE_DIVIDE)

I'd suggest to use
    if ((flag & (SANITIZE_SHIFT | SANITIZE_DIVIDE))
instead.

>        && !processing_template_decl
>        && current_function_decl != 0
>        && !lookup_attribute ("no_sanitize_undefined",
> @@ -4898,7 +4898,7 @@ cp_build_binary_op (location_t location,
>  								  tf_none));
>        op1 = maybe_constant_value (fold_non_dependent_expr_sfinae (op1,
>  								  tf_none));
> -      if (doing_div_or_mod)
> +      if (doing_div_or_mod && flag_sanitize & SANITIZE_DIVIDE)

And, while the operator precedence is right, I think it would be better
to use if (doing_div_or_mod && (flag_sanitize & SANITIZE_DIVIDE))

	Jakub
diff mbox

Patch

--- gcc/cp/typeck.c.mp	2013-09-18 14:00:14.303869196 +0200
+++ gcc/cp/typeck.c	2013-09-18 14:08:21.287770112 +0200
@@ -4884,7 +4884,7 @@  cp_build_binary_op (location_t location,
   if (build_type == NULL_TREE)
     build_type = result_type;
 
-  if ((flag_sanitize & SANITIZE_UNDEFINED)
+  if ((flag_sanitize & SANITIZE_SHIFT || flag_sanitize & SANITIZE_DIVIDE)
       && !processing_template_decl
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
@@ -4898,7 +4898,7 @@  cp_build_binary_op (location_t location,
 								  tf_none));
       op1 = maybe_constant_value (fold_non_dependent_expr_sfinae (op1,
 								  tf_none));
-      if (doing_div_or_mod)
+      if (doing_div_or_mod && flag_sanitize & SANITIZE_DIVIDE)
 	{
 	  /* For diagnostics we want to use the promoted types without
 	     shorten_binary_op.  So convert the arguments to the
@@ -4912,7 +4912,7 @@  cp_build_binary_op (location_t location,
 	    }
 	  instrument_expr = ubsan_instrument_division (location, cop0, cop1);
 	}
-      else if (doing_shift)
+      else if (doing_shift && flag_sanitize & SANITIZE_SHIFT)
 	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
     }
 
@@ -4926,7 +4926,7 @@  cp_build_binary_op (location_t location,
       && !TREE_OVERFLOW_P (op1))
     overflow_warning (location, result);
 
-  if ((flag_sanitize & SANITIZE_UNDEFINED) && instrument_expr != NULL)
+  if (instrument_expr != NULL)
     result = fold_build2 (COMPOUND_EXPR, TREE_TYPE (result),
 			  instrument_expr, result);
 
--- gcc/c/c-typeck.c.mp	2013-09-18 14:00:06.704837584 +0200
+++ gcc/c/c-typeck.c	2013-09-18 14:04:57.372968880 +0200
@@ -10496,7 +10496,7 @@  build_binary_op (location_t location, en
 	return error_mark_node;
     }
 
-  if (flag_sanitize & SANITIZE_UNDEFINED
+  if ((flag_sanitize & SANITIZE_SHIFT || flag_sanitize & SANITIZE_DIVIDE)
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
 			    DECL_ATTRIBUTES (current_function_decl))
@@ -10507,9 +10507,9 @@  build_binary_op (location_t location, en
       op1 = c_save_expr (op1);
       op0 = c_fully_fold (op0, false, NULL);
       op1 = c_fully_fold (op1, false, NULL);
-      if (doing_div_or_mod)
+      if (doing_div_or_mod && flag_sanitize & SANITIZE_DIVIDE)
 	instrument_expr = ubsan_instrument_division (location, op0, op1);
-      else if (doing_shift)
+      else if (doing_shift && flag_sanitize & SANITIZE_SHIFT)
 	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
     }
 
@@ -10537,7 +10537,7 @@  build_binary_op (location_t location, en
     ret = build1 (EXCESS_PRECISION_EXPR, semantic_result_type, ret);
   protected_set_expr_location (ret, location);
 
-  if ((flag_sanitize & SANITIZE_UNDEFINED) && instrument_expr != NULL)
+  if (instrument_expr != NULL)
     ret = fold_build2 (COMPOUND_EXPR, TREE_TYPE (ret),
 		       instrument_expr, ret);
 
--- gcc/testsuite/g++.dg/ubsan/div-by-zero-1.C.mp	2013-09-18 14:24:42.735312422 +0200
+++ gcc/testsuite/g++.dg/ubsan/div-by-zero-1.C	2013-09-18 14:25:28.965491706 +0200
@@ -1,5 +1,5 @@ 
 /* { dg-do compile } */
-/* { dg-options "-fsanitize=shift -w" } */
+/* { dg-options "-fsanitize=integer-divide-by-zero -w" } */
 
 void
 foo (int i)
--- gcc/testsuite/c-c++-common/ubsan/pr58443-1.c.mp	2013-09-18 14:20:18.633348043 +0200
+++ gcc/testsuite/c-c++-common/ubsan/pr58443-1.c	2013-09-18 14:30:45.403701302 +0200
@@ -0,0 +1,11 @@ 
+/* PR sanitizer/58443 */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift,unreachable -w" } */
+
+int
+foo (int u, int o)
+{
+  return u / o;
+}
+
+/* { dg-final { scan-assembler-not "__ubsan_handle_divrem_overflow" } } */
--- gcc/testsuite/c-c++-common/ubsan/pr58443-3.c.mp	2013-09-18 14:20:31.724398731 +0200
+++ gcc/testsuite/c-c++-common/ubsan/pr58443-3.c	2013-09-18 14:30:23.959623394 +0200
@@ -0,0 +1,18 @@ 
+/* PR sanitizer/58443 */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=undefined -w" } */
+
+int
+foo (int u, int o)
+{
+  return u >> o;
+}
+
+int
+bar (int u, int o)
+{
+  return u / o;
+}
+
+/* { dg-final { scan-assembler "__ubsan_handle_divrem_overflow" } } */
+/* { dg-final { scan-assembler "__ubsan_handle_shift_out_of_bounds" } } */
--- gcc/testsuite/c-c++-common/ubsan/pr58443-2.c.mp	2013-09-18 14:20:27.692384073 +0200
+++ gcc/testsuite/c-c++-common/ubsan/pr58443-2.c	2013-09-18 14:28:17.272155763 +0200
@@ -0,0 +1,11 @@ 
+/* PR sanitizer/58443 */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=unreachable,integer-divide-by-zero -w" } */
+
+int
+foo (int u, int o)
+{
+  return u >> o;
+}
+
+/* { dg-final { scan-assembler-not "__ubsan_handle_shift_out_of_bounds" } } */