diff mbox

C PATCH to fix ICE with old-style parameter declarations with initializers (PR sanitizer/79757)

Message ID 20170306214359.GV3172@redhat.com
State New
Headers show

Commit Message

Marek Polacek March 6, 2017, 9:44 p.m. UTC 
This is an ICE with -fsanitize=undefined on invalid code involving old-style
parameter declarations and nested functions.  Ugh.

Old-style parameter declarations cannot have initializers, and start_decl
errors when it sees that something's trying to initialize a PARM_DECL, but
nonetheless, we still attempt to parse the initializer, eventually arriving in
build_binary_op, which means we will instrument the bogus initializer.

This causes a crash in ubsan_encode_value -> create_tmp_var -> declare_vars: it
thinks it's processing a nested function, but the body of the function is null,
leading to the crash.

I suppose the best fix is to avoid instrumenting such bogus initializers.  In
c_parser_declaration_or_fndef it's easy to determine if we're initializing a
PARM_DECL -- just check DECL's code, but with __auto_type that's not possible,
because start_decl is called *after* parsing the initializer.  So instead of
DECL's code I'm checking nested && !empty_ok.

But we still want to instrument variables with initializers in nested
functions, so I've added two run-time tests for that.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-03-06  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/79757
	* c-parser.c (c_parser_declaration_or_fndef): Don't sanitize old-style
	parameter declarations with initializers.

	* gcc.dg/ubsan/pr79757-1.c: New test.
	* gcc.dg/ubsan/pr79757-2.c: New test.
	* gcc.dg/ubsan/pr79757-3.c: New test.
	* gcc.dg/ubsan/pr79757-4.c: New test.
	* gcc.dg/ubsan/pr79757-5.c: New test.


	Marek

Comments

Jeff Law March 7, 2017, 4:51 p.m. UTC | #1 
On 03/06/2017 02:44 PM, Marek Polacek wrote:
> This is an ICE with -fsanitize=undefined on invalid code involving old-style
> parameter declarations and nested functions.  Ugh.
>
> Old-style parameter declarations cannot have initializers, and start_decl
> errors when it sees that something's trying to initialize a PARM_DECL, but
> nonetheless, we still attempt to parse the initializer, eventually arriving in
> build_binary_op, which means we will instrument the bogus initializer.
>
> This causes a crash in ubsan_encode_value -> create_tmp_var -> declare_vars: it
> thinks it's processing a nested function, but the body of the function is null,
> leading to the crash.
>
> I suppose the best fix is to avoid instrumenting such bogus initializers.  In
> c_parser_declaration_or_fndef it's easy to determine if we're initializing a
> PARM_DECL -- just check DECL's code, but with __auto_type that's not possible,
> because start_decl is called *after* parsing the initializer.  So instead of
> DECL's code I'm checking nested && !empty_ok.
>
> But we still want to instrument variables with initializers in nested
> functions, so I've added two run-time tests for that.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2017-03-06  Marek Polacek  <polacek@redhat.com>
>
> 	PR sanitizer/79757
> 	* c-parser.c (c_parser_declaration_or_fndef): Don't sanitize old-style
> 	parameter declarations with initializers.
>
> 	* gcc.dg/ubsan/pr79757-1.c: New test.
> 	* gcc.dg/ubsan/pr79757-2.c: New test.
> 	* gcc.dg/ubsan/pr79757-3.c: New test.
> 	* gcc.dg/ubsan/pr79757-4.c: New test.
> 	* gcc.dg/ubsan/pr79757-5.c: New test.
This isn't marked as a regression, but it clearly is a regression given 
we didn't ICE prior to r205714.

OK for the trunk.

Thanks for adding tests that verify we are instrumenting initializers in 
nested functions.

jeff
diff mbox

Patch

diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index fa4e950..6ff023c 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -1859,7 +1859,13 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  init_loc = c_parser_peek_token (parser)->location;
 		  rich_location richloc (line_table, init_loc);
 		  start_init (NULL_TREE, asm_name, global_bindings_p (), &richloc);
+		  /* A parameter is initialized, which is invalid.  Don't
+		     attempt to instrument the initializer.  */
+		  int flag_sanitize_save = flag_sanitize;
+		  if (nested && !empty_ok)
+		    flag_sanitize = 0;
 		  init = c_parser_expr_no_commas (parser, NULL);
+		  flag_sanitize = flag_sanitize_save;
 		  if (TREE_CODE (init.value) == COMPONENT_REF
 		      && DECL_C_BIT_FIELD (TREE_OPERAND (init.value, 1)))
 		    error_at (here,
@@ -1917,7 +1923,13 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  init_loc = c_parser_peek_token (parser)->location;
 		  rich_location richloc (line_table, init_loc);
 		  start_init (d, asm_name, global_bindings_p (), &richloc);
+		  /* A parameter is initialized, which is invalid.  Don't
+		     attempt to instrument the initializer.  */
+		  int flag_sanitize_save = flag_sanitize;
+		  if (TREE_CODE (d) == PARM_DECL)
+		    flag_sanitize = 0;
 		  init = c_parser_initializer (parser);
+		  flag_sanitize = flag_sanitize_save;
 		  finish_init ();
 		}
 	      if (oacc_routine_data)
diff --git gcc/testsuite/gcc.dg/ubsan/pr79757-1.c gcc/testsuite/gcc.dg/ubsan/pr79757-1.c
index e69de29..ca074bc 100644
--- gcc/testsuite/gcc.dg/ubsan/pr79757-1.c
+++ gcc/testsuite/gcc.dg/ubsan/pr79757-1.c
@@ -0,0 +1,24 @@ 
+/* PR sanitizer/79757 */
+/* { dg-do compile } */
+/* { dg-require-effective-target int128 } */
+/* { dg-options "-fsanitize=undefined" } */
+
+unsigned __int128 x, y;
+
+void
+fn1 (void)
+{
+  int a (z)
+    unsigned long long z = x / y; /* { dg-error "parameter 'z' is initialized" } */
+  {
+  }
+}
+
+void
+fn2 (void)
+{
+  int a (z)
+    unsigned long long z = x >> y; /* { dg-error "parameter 'z' is initialized" } */
+  {
+  }
+}
diff --git gcc/testsuite/gcc.dg/ubsan/pr79757-2.c gcc/testsuite/gcc.dg/ubsan/pr79757-2.c
index e69de29..b3e1939 100644
--- gcc/testsuite/gcc.dg/ubsan/pr79757-2.c
+++ gcc/testsuite/gcc.dg/ubsan/pr79757-2.c
@@ -0,0 +1,18 @@ 
+/* PR sanitizer/79757 */
+/* { dg-do compile } */
+/* { dg-require-effective-target int128 } */
+/* { dg-options "-fsanitize=undefined" } */
+
+unsigned __int128 x, y;
+
+void
+fn1 (z)
+  unsigned long long z = x / y; /* { dg-error "parameter 'z' is initialized" } */
+{
+}
+
+void
+fn2 (z)
+  unsigned long long z = x >> y; /* { dg-error "parameter 'z' is initialized" } */
+{
+}
diff --git gcc/testsuite/gcc.dg/ubsan/pr79757-3.c gcc/testsuite/gcc.dg/ubsan/pr79757-3.c
index e69de29..22fe3de 100644
--- gcc/testsuite/gcc.dg/ubsan/pr79757-3.c
+++ gcc/testsuite/gcc.dg/ubsan/pr79757-3.c
@@ -0,0 +1,18 @@ 
+/* PR sanitizer/79757 */
+/* { dg-do compile } */
+/* { dg-require-effective-target int128 } */
+/* { dg-options "-fsanitize=undefined" } */
+
+unsigned __int128 x, y;
+
+void
+fn1 (z)
+  __auto_type z = x / y; /* { dg-error "parameter 'z' is initialized" } */
+{
+}
+
+void
+fn2 (z)
+  __auto_type z = x >> y; /* { dg-error "parameter 'z' is initialized" } */
+{
+}
diff --git gcc/testsuite/gcc.dg/ubsan/pr79757-4.c gcc/testsuite/gcc.dg/ubsan/pr79757-4.c
index e69de29..33b348f 100644
--- gcc/testsuite/gcc.dg/ubsan/pr79757-4.c
+++ gcc/testsuite/gcc.dg/ubsan/pr79757-4.c
@@ -0,0 +1,29 @@ 
+/* PR sanitizer/79757 */
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=undefined" } */
+
+int
+main (void)
+{
+  int
+  div (int n)
+  {
+    int i = 5 / n;
+    return i;
+  }
+
+  int
+  shift (int n)
+  {
+    int i = 5 << n;
+    return i;
+  }
+
+  int j = shift (100);
+  int i = div (0);
+  return 0;
+}
+
+/* { dg-output "shift exponent 100 is too large for \[^\n\r]*-bit type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero" } */
diff --git gcc/testsuite/gcc.dg/ubsan/pr79757-5.c gcc/testsuite/gcc.dg/ubsan/pr79757-5.c
index e69de29..786d817 100644
--- gcc/testsuite/gcc.dg/ubsan/pr79757-5.c
+++ gcc/testsuite/gcc.dg/ubsan/pr79757-5.c
@@ -0,0 +1,29 @@ 
+/* PR sanitizer/79757 */
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=undefined" } */
+
+int
+main (void)
+{
+  int
+  div (int n)
+  {
+    __auto_type i = 5 / n;
+    return i;
+  }
+
+  int
+  shift (int n)
+  {
+    __auto_type i = 5 << n;
+    return i;
+  }
+
+  int j = shift (100);
+  int i = div (0);
+  return 0;
+}
+
+/* { dg-output "shift exponent 100 is too large for \[^\n\r]*-bit type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero" } */