Re: [PATCH][ubsan] Add VLA bound instrumentation

[Marek Polacek <polacek@redhat.com>]

On Thu, Oct 24, 2013 at 03:57:17PM -0400, Jason Merrill wrote:
> On 09/25/2013 08:41 AM, Marek Polacek wrote:
> >+  /* Do the instrumentation of VLAs if desired.  */
> >+  if ((flag_sanitize & SANITIZE_VLA)
> >+      && size && !TREE_CONSTANT (size)
> >+      /* From C++1y onwards, we throw an exception on a negative length size
> >+         of an array.  */
> >+      && cxx_dialect < cxx1y)
> 
> This code is in a completely different place from the C++1y code in
> cp_finish_decl; they should be in the same place.  I'm also
> concerned that doing it here will mean adding sanitization code to
> template definitions, but I think we want to wait to add it until
> instantiation time.
 
I've tried to implement the instrumentation in cp_finish_decl.
However, the problem is with multidimensional arrays, e.g. for

int x = -1;
int a[1][x];

array_of_runtime_bound_p returns false, thus we don't instrument this
at all, nor throw an exception in c++1y mode...  I don't know what to
do with that.  Previous implementation in create_array_type_for_decl
handled this fine.

> >+      /* Prevent bogus set-but-not-used warnings: we're definitely using
> >+         the variable.  */
> >+      if (VAR_P (size))
> >+        DECL_READ_P (size) = 1;
> 
> Use mark_rvalue_use for this.

Ah, thanks.  This is not needed anymore.

Both ubsan testsuite + bootstrap-ubsan pass.

2013-10-25  Marek Polacek  <polacek@redhat.com>

	Implement -fsanitize=vla-bound.
	* opts.c (common_handle_option): Handle vla-bound.
	* sanitizer.def (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE):
	Define.
	* flag-types.h (enum sanitize_code): Add SANITIZE_VLA.
	* asan.c (initialize_sanitizer_builtins): Build BT_FN_VOID_PTR_PTR.
c-family/
	* c-ubsan.c: Don't include hash-table.h.
	(ubsan_instrument_vla): New function.
	* c-ubsan.h: Declare it.
cp/
	* decl.c (cp_finish_decl): Add VLA instrumentation.
c/
	* c-decl.c (grokdeclarator): Add VLA instrumentation.
testsuite/
	* g++.dg/ubsan/cxx1y-vla.C: New test.
	* c-c++-common/ubsan/vla-3.c: New test.
	* c-c++-common/ubsan/vla-2.c: New test.
	* c-c++-common/ubsan/vla-4.c: New test.
	* c-c++-common/ubsan/vla-1.c: New test.

--- gcc/opts.c.mp	2013-10-25 11:56:57.697200002 +0200
+++ gcc/opts.c	2013-10-25 11:57:04.221224139 +0200
@@ -1445,6 +1445,7 @@ common_handle_option (struct gcc_options
 	      { "undefined", SANITIZE_UNDEFINED, sizeof "undefined" - 1 },
 	      { "unreachable", SANITIZE_UNREACHABLE,
 		sizeof "unreachable" - 1 },
+	      { "vla-bound", SANITIZE_VLA, sizeof "vla-bound" - 1 },
 	      { NULL, 0, 0 }
 	    };
 	    const char *comma;
--- gcc/c-family/c-ubsan.c.mp	2013-10-25 11:56:57.699200012 +0200
+++ gcc/c-family/c-ubsan.c	2013-10-25 11:57:04.223224148 +0200
@@ -25,7 +25,6 @@ along with GCC; see the file COPYING3.
 #include "alloc-pool.h"
 #include "cgraph.h"
 #include "gimple.h"
-#include "hash-table.h"
 #include "output.h"
 #include "toplev.h"
 #include "ubsan.h"
@@ -86,8 +85,7 @@ ubsan_instrument_division (location_t lo
   return t;
 }
 
-/* Instrument left and right shifts.  If not instrumenting, return
-   NULL_TREE.  */
+/* Instrument left and right shifts.  */
 
 tree
 ubsan_instrument_shift (location_t loc, enum tree_code code,
@@ -157,4 +155,23 @@ ubsan_instrument_shift (location_t loc,
   t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_zero_node);
 
   return t;
+}
+
+/* Instrument variable length array bound.  */
+
+tree
+ubsan_instrument_vla (location_t loc, tree size)
+{
+  tree type = TREE_TYPE (size);
+  tree t, tt;
+
+  t = fold_build2 (LE_EXPR, boolean_type_node, size, build_int_cst (type, 0));
+  tree data = ubsan_create_data ("__ubsan_vla_data",
+				 loc, ubsan_type_descriptor (type), NULL_TREE);
+  data = build_fold_addr_expr_loc (loc, data);
+  tt = builtin_decl_explicit (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE);
+  tt = build_call_expr_loc (loc, tt, 2, data, ubsan_encode_value (size));
+  t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_zero_node);
+
+  return t;
 }
--- gcc/c-family/c-ubsan.h.mp	2013-10-25 11:56:57.700200016 +0200
+++ gcc/c-family/c-ubsan.h	2013-10-25 11:57:04.223224148 +0200
@@ -23,5 +23,6 @@ along with GCC; see the file COPYING3.
 
 extern tree ubsan_instrument_division (location_t, tree, tree);
 extern tree ubsan_instrument_shift (location_t, enum tree_code, tree, tree);
+extern tree ubsan_instrument_vla (location_t, tree);
 
 #endif  /* GCC_C_UBSAN_H  */
--- gcc/sanitizer.def.mp	2013-10-25 11:56:57.703200028 +0200
+++ gcc/sanitizer.def	2013-10-25 11:57:04.225224158 +0200
@@ -297,3 +297,7 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HAN
 		      "__ubsan_handle_builtin_unreachable",
 		      BT_FN_VOID_PTR,
 		      ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE,
+		      "__ubsan_handle_vla_bound_not_positive",
+		      BT_FN_VOID_PTR_PTR,
+		      ATTR_COLD_NOTHROW_LEAF_LIST)
--- gcc/flag-types.h.mp	2013-10-25 11:56:57.705200036 +0200
+++ gcc/flag-types.h	2013-10-25 11:57:04.228224170 +0200
@@ -210,7 +210,9 @@ enum sanitize_code {
   SANITIZE_SHIFT = 1 << 2,
   SANITIZE_DIVIDE = 1 << 3,
   SANITIZE_UNREACHABLE = 1 << 4,
+  SANITIZE_VLA = 1 << 5,
   SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE
+		       | SANITIZE_VLA
 };
 
 /* flag_vtable_verify initialization levels. */
--- gcc/cp/decl.c.mp	2013-10-25 11:56:57.707200046 +0200
+++ gcc/cp/decl.c	2013-10-25 18:06:48.122207862 +0200
@@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
 #include "c-family/c-objc.h"
 #include "c-family/c-pragma.h"
 #include "c-family/c-target.h"
+#include "c-family/c-ubsan.h"
 #include "diagnostic.h"
 #include "intl.h"
 #include "debug.h"
@@ -6399,6 +6400,22 @@ cp_finish_decl (tree decl, tree init, bo
 	   && TYPE_FOR_JAVA (type) && MAYBE_CLASS_TYPE_P (type))
     error ("non-static data member %qD has Java class type", decl);
 
+  if ((flag_sanitize & SANITIZE_VLA)
+      && array_of_runtime_bound_p (type)
+      && !processing_template_decl
+      /* From C++1y onwards, we throw an exception on a negative length size
+         of an array.  */
+      && cxx_dialect < cxx1y)
+    {
+      tree t = convert (ssizetype, TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
+      /* We have to add 1.  */
+      t = fold_build2 (PLUS_EXPR, TREE_TYPE (t), t,
+		       build_one_cst (TREE_TYPE (t)));
+      t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (type),
+		       ubsan_instrument_vla (input_location, t), t);
+      finish_expr_stmt (t);
+    }
+
   if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
     {
       /* If the VLA bound is larger than half the address space, or less
--- gcc/c/c-decl.c.mp	2013-10-25 11:56:57.709200056 +0200
+++ gcc/c/c-decl.c	2013-10-25 11:57:04.242224233 +0200
@@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
 #include "c-family/c-common.h"
 #include "c-family/c-objc.h"
 #include "c-family/c-pragma.h"
+#include "c-family/c-ubsan.h"
 #include "c-lang.h"
 #include "langhooks.h"
 #include "tree-iterator.h"
@@ -5410,6 +5411,16 @@ grokdeclarator (const struct c_declarato
 		       with known value.  */
 		    this_size_varies = size_varies = true;
 		    warn_variable_length_array (name, size);
+		    if (flag_sanitize & SANITIZE_VLA
+		        && decl_context == NORMAL)
+		      {
+			/* Evaluate the array size only once.  */
+			size = c_save_expr (size);
+			size = c_fully_fold (size, false, NULL);
+		        size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
+					    ubsan_instrument_vla (loc, size),
+					    size);
+		      }
 		  }
 
 		if (integer_zerop (size) && !this_size_varies)
--- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-10-25 11:57:15.022264290 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-10-25 11:57:04.252224277 +0200
@@ -0,0 +1,16 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w" } */
+
+/* Don't instrument the arrays here.  */
+int
+foo (int n, int a[])
+{
+  return a[n - 1];
+}
+
+int
+main (void)
+{
+  int a[6] = { };
+  return foo (3, a);
+}
--- gcc/testsuite/c-c++-common/ubsan/vla-2.c.mp	2013-10-25 11:57:13.556258679 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-10-25 11:57:04.252224277 +0200
@@ -0,0 +1,15 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w" } */
+
+int
+main (void)
+{
+  const int t = 0;
+  struct s {
+    int x;
+    /* Don't instrument this one.  */
+    int g[t];
+  };
+
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/vla-4.c.mp	2013-10-25 11:57:09.966245168 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-10-25 11:57:04.253224281 +0200
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound" } */
+
+int
+main (void)
+{
+  int x = 1;
+  /* Check that the size of an array is evaluated only once.  */
+  int a[++x];
+  if (x != 2)
+    __builtin_abort ();
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/vla-1.c.mp	2013-10-25 11:57:11.813252061 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-10-25 17:48:46.292816847 +0200
@@ -0,0 +1,41 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w" } */
+
+static int
+bar (void)
+{
+  return -42;
+}
+
+typedef long int V;
+int
+main (void)
+{
+  int x = -1;
+  double di = -3.2;
+  V v = -666;
+
+  int a[x];
+  int b[x - 4];
+  int c[(int) di];
+  int d[1 + x];
+  int e[1 ? x : -1];
+  int f[++x];
+  int g[(signed char) --x];
+  int h[(++x, --x, x)];
+  int i[v];
+  int j[bar ()];
+
+  return 0;
+}
+
+/* { dg-output "variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -5(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -3(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -666(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -42(\n|\r\n|\r)" } */
--- gcc/asan.c.mp	2013-10-25 11:56:57.715200082 +0200
+++ gcc/asan.c	2013-10-25 11:57:04.254224285 +0200
@@ -2021,6 +2021,9 @@ initialize_sanitizer_builtins (void)
   tree BT_FN_VOID = build_function_type_list (void_type_node, NULL_TREE);
   tree BT_FN_VOID_PTR
     = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
+  tree BT_FN_VOID_PTR_PTR
+    = build_function_type_list (void_type_node, ptr_type_node,
+				ptr_type_node, NULL_TREE);
   tree BT_FN_VOID_PTR_PTR_PTR
     = build_function_type_list (void_type_node, ptr_type_node,
 				ptr_type_node, ptr_type_node, NULL_TREE);

	Marek