[PATCH][ubsan] Add VLA bound instrumentation

[PATCH][ubsan] Add VLA bound instrumentation

[Marek Polacek]

This patch adds the instrumentation of VLA bounds.  Basically, it just checks that
the size of a VLA is positive.  I.e., We also issue an error if the size of the
VLA is 0.  It catches e.g.

int i = 1;
int a[i][i - 2];

It is pretty straightforward, but I had
issues in the C++ FE, mainly choosing the right spot where to instrument...
Hopefully I picked up the right one.  Also note that in C++1y we throw
an exception when the size of a VLA is negative; hence no need to perform
the instrumentation if -std=c++1y is in effect.

Regtested/ran bootstrap-ubsan on x86_64-linux, also
make check -C gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} ubsan.exp'
passes.

Ok for trunk?

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

	* opts.c (common_handle_option): Handle vla-bound.
	* sanitizer.def (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE):
	Define.
	* ubsan.c (ubsan_type_descriptor): Handle IDENTIFIER_NODEs.
	* 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 (create_array_type_for_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-09-12 13:30:53.299113222 +0200
+++ gcc/opts.c	2013-09-12 13:31:31.496263290 +0200
@@ -1426,6 +1426,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-09-12 13:30:17.306967720 +0200
+++ gcc/c-family/c-ubsan.c	2013-09-12 13:31:31.469263169 +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"
@@ -89,8 +88,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,
@@ -155,4 +153,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-09-12 13:30:25.609000661 +0200
+++ gcc/c-family/c-ubsan.h	2013-09-12 13:31:31.475263194 +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-09-12 13:30:59.667138181 +0200
+++ gcc/sanitizer.def	2013-09-12 13:31:31.497263294 +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/ubsan.c.mp	2013-09-12 13:31:06.197163836 +0200
+++ gcc/ubsan.c	2013-09-12 13:31:31.498263298 +0200
@@ -261,7 +261,9 @@ ubsan_type_descriptor (tree type)
 
   /* At least for INTEGER_TYPE/REAL_TYPE/COMPLEX_TYPE, this should work.
      ??? For e.g. type_unsigned_for (type), the TYPE_NAME would be NULL.  */
-  if (TYPE_NAME (type) != NULL)
+  if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE)
+    tname = IDENTIFIER_POINTER (TYPE_NAME (type));
+  else if (TYPE_NAME (type) != NULL)
     tname = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type)));
   else
     tname = "<unknown>";
--- gcc/flag-types.h.mp	2013-09-12 13:30:47.130090269 +0200
+++ gcc/flag-types.h	2013-09-12 13:31:31.495263285 +0200
@@ -201,7 +201,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-09-12 13:30:39.641060204 +0200
+++ gcc/cp/decl.c	2013-09-12 13:31:31.488263253 +0200
@@ -44,6 +44,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 "diagnostic.h"
 #include "intl.h"
 #include "debug.h"
@@ -8462,6 +8463,24 @@ create_array_type_for_decl (tree name, t
   if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
     pedwarn (input_location, OPT_Wvla, "array of array of runtime bound");
 
+  /* 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)
+    {
+      /* Prevent bogus set-but-not-used warnings: we're definitely using
+         the variable.  */
+      if (VAR_P (size))
+        DECL_READ_P (size) = 1;
+      /* Evaluate the array size only once.  */
+      size = cp_save_expr (size);
+      size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
+			  ubsan_instrument_vla (input_location, size),
+			  size);
+    }
+
   /* Figure out the index type for the array.  */
   if (size)
     itype = compute_array_index_type (name, size, tf_warning_or_error);
--- gcc/c/c-decl.c.mp	2013-09-12 13:30:32.352029153 +0200
+++ gcc/c/c-decl.c	2013-09-12 13:31:31.478263209 +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"
@@ -5381,6 +5382,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-09-12 13:17:55.242089503 +0200
+++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-09-12 13:27:38.649460187 +0200
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
+/* { dg-shouldfail "ubsan" } */
+
+int
+main (void)
+{
+  int y = -18;
+  int a[y];
+  return 0;
+}
+
+/* { dg-output "terminate called after throwing an instance" } */
--- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-09-12 10:48:11.719745997 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-09-12 12:06:43.178724666 +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-09-12 10:47:56.662693753 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-09-12 12:06:28.698670292 +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-09-12 10:48:14.023754028 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-09-12 12:00:37.639137936 +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-09-12 10:47:54.377685875 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-09-12 11:00:37.693810414 +0200
@@ -0,0 +1,48 @@
+/* { 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 aa[x][x];
+  int aaa[x][x][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 -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 -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 -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-09-12 13:30:10.530941672 +0200
+++ gcc/asan.c	2013-09-12 13:31:31.469263169 +0200
@@ -2018,6 +2018,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

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote:
> the size of a VLA is positive.  I.e., We also issue an error if the size of the

s/We also/we/

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

[Joseph S. Myers]

On Thu, 12 Sep 2013, Marek Polacek wrote:

> This patch adds the instrumentation of VLA bounds.  Basically, it just 
> checks that the size of a VLA is positive.  I.e., We also issue an error 
> if the size of the VLA is 0.  It catches e.g.

This is not an objection to this patch, but there are a few other bits of 
VLA bound instrumentation that could be done as well.  If the size has a 
wide-enough type to be bigger than the target's SIZE_MAX, and is indeed 
bigger than SIZE_MAX, that could be detected at runtime as well.  Or if 
the multiplication of array size and element size exceeds SIZE_MAX (this 
covers both elements of constant size, and elements that are themselves 
VLAs, but the former can be handled more efficiently by comparing against 
an appropriate constant rather than needing a runtime test for whether a 
multiplication in size_t overflows).

(Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not 
just SIZE_MAX, should be caught, because pointer subtraction cannot work 
reliably with larger objects.  So it's not just when the size or 
multiplication overflow size_t, but when they overflow ptrdiff_t.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Joseph S. Myers]

On Thu, 12 Sep 2013, Joseph S. Myers wrote:

> (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not 
> just SIZE_MAX, should be caught, because pointer subtraction cannot work 
> reliably with larger objects.  So it's not just when the size or 
> multiplication overflow size_t, but when they overflow ptrdiff_t.)

And, to add a bit more to the list of possible ubsan features (is this 
todo list maintained anywhere?), even if the size is such that operations 
on the array are in principle defined, it's possible that adjusting the 
stack pointer by too much may take it into other areas of memory and so 
cause stack overflow that doesn't get detected by the kernel.  So maybe 
ubsan should imply -fstack-check or similar.

Everything about VLA checking - checks on the size being positive and on 
it not being larger than PTRDIFF_MAX, and on avoiding stack overflow - 
applies equally to alloca: calls to alloca should also be instrumented.  
(But I think alloca (0) is valid.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 03:52:18PM +0000, Joseph S. Myers wrote:
> On Thu, 12 Sep 2013, Marek Polacek wrote:
> 
> > This patch adds the instrumentation of VLA bounds.  Basically, it just 
> > checks that the size of a VLA is positive.  I.e., We also issue an error 
> > if the size of the VLA is 0.  It catches e.g.
> 
> This is not an objection to this patch, but there are a few other bits of 
> VLA bound instrumentation that could be done as well.  If the size has a 
> wide-enough type to be bigger than the target's SIZE_MAX, and is indeed 
> bigger than SIZE_MAX, that could be detected at runtime as well.  Or if 
> the multiplication of array size and element size exceeds SIZE_MAX (this 
> covers both elements of constant size, and elements that are themselves 
> VLAs, but the former can be handled more efficiently by comparing against 
> an appropriate constant rather than needing a runtime test for whether a 
> multiplication in size_t overflows).
> 
> (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not 
> just SIZE_MAX, should be caught, because pointer subtraction cannot work 
> reliably with larger objects.  So it's not just when the size or 
> multiplication overflow size_t, but when they overflow ptrdiff_t.)

Yup, this all sounds good.  I'll look at this tomorrow.  I think I'd
prefer doing this as a follow-up, after the C/C++ FE parts are
reviewed; doing SIZE_MAX/PTRDIFF_MAX checking then should require
changes only in c-ubsan.c.

	Marek

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 04:05:48PM +0000, Joseph S. Myers wrote:
> On Thu, 12 Sep 2013, Joseph S. Myers wrote:
> 
> > (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not 
> > just SIZE_MAX, should be caught, because pointer subtraction cannot work 
> > reliably with larger objects.  So it's not just when the size or 
> > multiplication overflow size_t, but when they overflow ptrdiff_t.)
> 
> And, to add a bit more to the list of possible ubsan features (is this 
> todo list maintained anywhere?), even if the size is such that operations 

No, I don't have such a list (at least nothing online/public).

> on the array are in principle defined, it's possible that adjusting the 
> stack pointer by too much may take it into other areas of memory and so 
> cause stack overflow that doesn't get detected by the kernel.  So maybe 
> ubsan should imply -fstack-check or similar.

Works for me.

> Everything about VLA checking - checks on the size being positive and on 
> it not being larger than PTRDIFF_MAX, and on avoiding stack overflow - 
> applies equally to alloca: calls to alloca should also be instrumented.  
> (But I think alloca (0) is valid.)

Yes, good idea.  I've just checked and clang doesn't check the size
passed to alloca, I think it'd be good addition to have it.

And yeah - alloca (0) seems to be valid; when expanding
__builtin_alloca we call allocate_dynamic_stack_space and that
contains

  /* If we're asking for zero bytes, it doesn't matter what we point
     to since we can't dereference it.  But return a reasonable
     address anyway.  */
  if (size == const0_rtx)
    return virtual_stack_dynamic_rtx;

Thanks,

	Marek

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 04:05:48PM +0000, Joseph S. Myers wrote:
> cause stack overflow that doesn't get detected by the kernel.  So maybe 
> ubsan should imply -fstack-check or similar.

Well, I have a patch for that, but I no longer think that ubsan should
imply -fstack-check, since e.g. 

int
main (void)
{
  int x = -1;
  int b[x - 4];
  /* ... */
  return 0;
}

segfaults at runtime on int b[x - 4]; line when -fstack-check is used
(even without sanitizing), so we wouldn't give proper diagnostics
for stmts following that line...

	Marek

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

[Eric Botcazou]

> Well, I have a patch for that, but I no longer think that ubsan should
> imply -fstack-check, since e.g.
> 
> int
> main (void)
> {
>   int x = -1;
>   int b[x - 4];
>   /* ... */
>   return 0;
> }
> 
> segfaults at runtime on int b[x - 4]; line when -fstack-check is used
> (even without sanitizing), so we wouldn't give proper diagnostics
> for stmts following that line...

In Ada we catch the sigsegv, turn it into an exception and unwind.

-- 
Eric Botcazou

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

[Joseph S. Myers]

On Fri, 13 Sep 2013, Marek Polacek wrote:

> On Thu, Sep 12, 2013 at 04:05:48PM +0000, Joseph S. Myers wrote:
> > cause stack overflow that doesn't get detected by the kernel.  So maybe 
> > ubsan should imply -fstack-check or similar.
> 
> Well, I have a patch for that, but I no longer think that ubsan should
> imply -fstack-check, since e.g. 
> 
> int
> main (void)
> {
>   int x = -1;
>   int b[x - 4];
>   /* ... */
>   return 0;
> }
> 
> segfaults at runtime on int b[x - 4]; line when -fstack-check is used
> (even without sanitizing), so we wouldn't give proper diagnostics
> for stmts following that line...

A guaranteed segfault is better than doing something undefined.  But I'd 
expect sanitizing to make the initial check that the array size in bytes 
is in the range [1, PTRDIFF_MAX] and -fstack-check only to come into play 
if that passes (for sizes that are too large for the stack limit in effect 
at runtime although within the range that is in principle valid).  You 
probably don't want to enable -fstack-check from ubsan until the checks 
for the range [1, PTRDIFF_MAX] are in place.

(Those checks, incidentally, would need to apply not just to arrays whose 
specified size is variable, but also to constant-size arrays of 
variable-size arrays - if you have a VLA type, then define an array 
VLA array[10]; then you need to check that the result of the 
multiplication of sizes in bytes doesn't exceed PTRDIFF_MAX.  So the more 
general checks can't all go in the place where you're inserting the checks 
for a single variable size in isolation.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 04:05:48PM +0000, Joseph S. Myers wrote:
> On Thu, 12 Sep 2013, Joseph S. Myers wrote:
> 
> > (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not 
> > just SIZE_MAX, should be caught, because pointer subtraction cannot work 
> > reliably with larger objects.  So it's not just when the size or 
> > multiplication overflow size_t, but when they overflow ptrdiff_t.)
> 
> And, to add a bit more to the list of possible ubsan features (is this 
> todo list maintained anywhere?), even if the size is such that operations 
> on the array are in principle defined, it's possible that adjusting the 
> stack pointer by too much may take it into other areas of memory and so 
> cause stack overflow that doesn't get detected by the kernel.  So maybe 
> ubsan should imply -fstack-check or similar.
> 
> Everything about VLA checking - checks on the size being positive and on 
> it not being larger than PTRDIFF_MAX, and on avoiding stack overflow - 
> applies equally to alloca: calls to alloca should also be instrumented.  
> (But I think alloca (0) is valid.)

Problem here is that libubsan doesn't contain appropriate routines for
this VLA/alloca extended checking, it really can only issue "variable
length array bound evaluates to non-positive value", nothing else.

So perhaps reach out to some clang mailing list and try to implement
it first in the libubsan...

	Marek

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

[Florian Weimer]

On 09/12/2013 06:05 PM, Joseph S. Myers wrote:
> On Thu, 12 Sep 2013, Joseph S. Myers wrote:
>
>> (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not
>> just SIZE_MAX, should be caught, because pointer subtraction cannot work
>> reliably with larger objects.  So it's not just when the size or
>> multiplication overflow size_t, but when they overflow ptrdiff_t.)
>
> And, to add a bit more to the list of possible ubsan features (is this
> todo list maintained anywhere?), even if the size is such that operations
> on the array are in principle defined, it's possible that adjusting the
> stack pointer by too much may take it into other areas of memory and so
> cause stack overflow that doesn't get detected by the kernel.  So maybe
> ubsan should imply -fstack-check or similar.

I have on my to-do list to make -fstack-check production-ready, by 
avoiding unnecessary instrumentation.  My initial experiments weren't 
too successful, but I think it should be possible to greatly reduce its 
overhead.  If everything else fails, the idea is to reuse the Go split 
stack limit and check against that.

The idea is that this would eventually be enabled in production code, 
much like -fstack-protector.

I'm quite busy with other work at the moment, and a patch from me is 
probably months away, though. :-(

-- 
Florian Weimer / Red Hat Product Security Team

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

[Marek Polacek]

On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote:
> This patch adds the instrumentation of VLA bounds.  Basically, it just checks that
> the size of a VLA is positive.  I.e., We also issue an error if the size of the
> VLA is 0.  It catches e.g.
> 
> int i = 1;
> int a[i][i - 2];
> 
> It is pretty straightforward, but I had
> issues in the C++ FE, mainly choosing the right spot where to instrument...
> Hopefully I picked up the right one.  Also note that in C++1y we throw
> an exception when the size of a VLA is negative; hence no need to perform
> the instrumentation if -std=c++1y is in effect.
> 
> Regtested/ran bootstrap-ubsan on x86_64-linux, also
> make check -C gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} ubsan.exp'
> passes.
> 
> Ok for trunk?

I'd like to ping this patch; below is rebased version with the ubsan.c
hunk omitted, since that part was already fixed by another patch.

(It still doesn't contain alloca/SIZE_MAX/... checking, since that
very much relies on libubsan.  Still, it'd be felicitous to get at
least the basic VLA checking in.)

Ran ubsan testsuite + bootstrap-ubsan on x86_64-linux.

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

	* 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 (create_array_type_for_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-09-25 14:06:58.531276511 +0200
+++ gcc/opts.c	2013-09-25 14:07:03.580294566 +0200
@@ -1428,6 +1428,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-09-25 14:06:58.535276527 +0200
+++ gcc/c-family/c-ubsan.c	2013-09-25 14:07:03.580294566 +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-09-25 14:06:58.538276539 +0200
+++ gcc/c-family/c-ubsan.h	2013-09-25 14:07:03.595294628 +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-09-25 14:06:58.542276558 +0200
+++ gcc/sanitizer.def	2013-09-25 14:07:03.628294753 +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-09-25 14:06:58.546276575 +0200
+++ gcc/flag-types.h	2013-09-25 14:07:03.629294757 +0200
@@ -201,7 +201,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-09-25 14:06:58.549276587 +0200
+++ gcc/cp/decl.c	2013-09-25 14:07:20.640355737 +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"
@@ -8465,6 +8466,24 @@ create_array_type_for_decl (tree name, t
   if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
     pedwarn (input_location, OPT_Wvla, "array of array of runtime bound");
 
+  /* 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)
+    {
+      /* Prevent bogus set-but-not-used warnings: we're definitely using
+         the variable.  */
+      if (VAR_P (size))
+        DECL_READ_P (size) = 1;
+      /* Evaluate the array size only once.  */
+      size = cp_save_expr (size);
+      size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
+			  ubsan_instrument_vla (input_location, size),
+			  size);
+    }
+
   /* Figure out the index type for the array.  */
   if (size)
     itype = compute_array_index_type (name, size, tf_warning_or_error);
--- gcc/c/c-decl.c.mp	2013-09-25 14:06:58.550276591 +0200
+++ gcc/c/c-decl.c	2013-09-25 14:07:03.644294820 +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"
@@ -5378,6 +5379,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-09-25 14:08:33.263616709 +0200
+++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-09-25 14:07:03.650294845 +0200
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
+/* { dg-shouldfail "ubsan" } */
+
+int
+main (void)
+{
+  int y = -18;
+  int a[y];
+  return 0;
+}
+
+/* { dg-output "terminate called after throwing an instance" } */
--- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-09-25 14:08:25.364588140 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-09-25 14:07:03.650294845 +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-09-25 14:08:23.458581265 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-09-25 14:07:03.651294849 +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-09-25 14:08:27.367595369 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-09-25 14:07:03.652294853 +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-09-25 14:08:21.341573677 +0200
+++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-09-25 14:07:03.652294853 +0200
@@ -0,0 +1,48 @@
+/* { 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 aa[x][x];
+  int aaa[x][x][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 -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 -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 -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-09-25 14:06:58.557276623 +0200
+++ gcc/asan.c	2013-09-25 14:07:03.653294857 +0200
@@ -2018,6 +2018,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

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

[Marek Polacek]

Ping.

On Wed, Sep 25, 2013 at 02:41:32PM +0200, Marek Polacek wrote:
> On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote:
> > This patch adds the instrumentation of VLA bounds.  Basically, it just checks that
> > the size of a VLA is positive.  I.e., We also issue an error if the size of the
> > VLA is 0.  It catches e.g.
> > 
> > int i = 1;
> > int a[i][i - 2];
> > 
> > It is pretty straightforward, but I had
> > issues in the C++ FE, mainly choosing the right spot where to instrument...
> > Hopefully I picked up the right one.  Also note that in C++1y we throw
> > an exception when the size of a VLA is negative; hence no need to perform
> > the instrumentation if -std=c++1y is in effect.
> > 
> > Regtested/ran bootstrap-ubsan on x86_64-linux, also
> > make check -C gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} ubsan.exp'
> > passes.
> > 
> > Ok for trunk?
> 
> I'd like to ping this patch; below is rebased version with the ubsan.c
> hunk omitted, since that part was already fixed by another patch.
> 
> (It still doesn't contain alloca/SIZE_MAX/... checking, since that
> very much relies on libubsan.  Still, it'd be felicitous to get at
> least the basic VLA checking in.)
> 
> Ran ubsan testsuite + bootstrap-ubsan on x86_64-linux.
> 
> 2013-09-25  Marek Polacek  <polacek@redhat.com>
> 
> 	* 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 (create_array_type_for_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-09-25 14:06:58.531276511 +0200
> +++ gcc/opts.c	2013-09-25 14:07:03.580294566 +0200
> @@ -1428,6 +1428,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-09-25 14:06:58.535276527 +0200
> +++ gcc/c-family/c-ubsan.c	2013-09-25 14:07:03.580294566 +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-09-25 14:06:58.538276539 +0200
> +++ gcc/c-family/c-ubsan.h	2013-09-25 14:07:03.595294628 +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-09-25 14:06:58.542276558 +0200
> +++ gcc/sanitizer.def	2013-09-25 14:07:03.628294753 +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-09-25 14:06:58.546276575 +0200
> +++ gcc/flag-types.h	2013-09-25 14:07:03.629294757 +0200
> @@ -201,7 +201,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-09-25 14:06:58.549276587 +0200
> +++ gcc/cp/decl.c	2013-09-25 14:07:20.640355737 +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"
> @@ -8465,6 +8466,24 @@ create_array_type_for_decl (tree name, t
>    if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
>      pedwarn (input_location, OPT_Wvla, "array of array of runtime bound");
>  
> +  /* 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)
> +    {
> +      /* Prevent bogus set-but-not-used warnings: we're definitely using
> +         the variable.  */
> +      if (VAR_P (size))
> +        DECL_READ_P (size) = 1;
> +      /* Evaluate the array size only once.  */
> +      size = cp_save_expr (size);
> +      size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
> +			  ubsan_instrument_vla (input_location, size),
> +			  size);
> +    }
> +
>    /* Figure out the index type for the array.  */
>    if (size)
>      itype = compute_array_index_type (name, size, tf_warning_or_error);
> --- gcc/c/c-decl.c.mp	2013-09-25 14:06:58.550276591 +0200
> +++ gcc/c/c-decl.c	2013-09-25 14:07:03.644294820 +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"
> @@ -5378,6 +5379,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-09-25 14:08:33.263616709 +0200
> +++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-09-25 14:07:03.650294845 +0200
> @@ -0,0 +1,13 @@
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int
> +main (void)
> +{
> +  int y = -18;
> +  int a[y];
> +  return 0;
> +}
> +
> +/* { dg-output "terminate called after throwing an instance" } */
> --- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-09-25 14:08:25.364588140 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-09-25 14:07:03.650294845 +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-09-25 14:08:23.458581265 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-09-25 14:07:03.651294849 +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-09-25 14:08:27.367595369 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-09-25 14:07:03.652294853 +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-09-25 14:08:21.341573677 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-09-25 14:07:03.652294853 +0200
> @@ -0,0 +1,48 @@
> +/* { 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 aa[x][x];
> +  int aaa[x][x][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 -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 -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 -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-09-25 14:06:58.557276623 +0200
> +++ gcc/asan.c	2013-09-25 14:07:03.653294857 +0200
> @@ -2018,6 +2018,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

	Marek

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

[Marek Polacek]

Ping^2.  Jason, Joseph, are you fine with the C++/C FE changes?

Thanks.

On Mon, Oct 07, 2013 at 10:17:38PM +0200, Marek Polacek wrote:
> Ping.
> 
> On Wed, Sep 25, 2013 at 02:41:32PM +0200, Marek Polacek wrote:
> > On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote:
> > > This patch adds the instrumentation of VLA bounds.  Basically, it just checks that
> > > the size of a VLA is positive.  I.e., We also issue an error if the size of the
> > > VLA is 0.  It catches e.g.
> > > 
> > > int i = 1;
> > > int a[i][i - 2];
> > > 
> > > It is pretty straightforward, but I had
> > > issues in the C++ FE, mainly choosing the right spot where to instrument...
> > > Hopefully I picked up the right one.  Also note that in C++1y we throw
> > > an exception when the size of a VLA is negative; hence no need to perform
> > > the instrumentation if -std=c++1y is in effect.
> > > 
> > > Regtested/ran bootstrap-ubsan on x86_64-linux, also
> > > make check -C gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} ubsan.exp'
> > > passes.
> > > 
> > > Ok for trunk?
> > 
> > I'd like to ping this patch; below is rebased version with the ubsan.c
> > hunk omitted, since that part was already fixed by another patch.
> > 
> > (It still doesn't contain alloca/SIZE_MAX/... checking, since that
> > very much relies on libubsan.  Still, it'd be felicitous to get at
> > least the basic VLA checking in.)
> > 
> > Ran ubsan testsuite + bootstrap-ubsan on x86_64-linux.
> > 
> > 2013-09-25  Marek Polacek  <polacek@redhat.com>
> > 
> > 	* 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 (create_array_type_for_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-09-25 14:06:58.531276511 +0200
> > +++ gcc/opts.c	2013-09-25 14:07:03.580294566 +0200
> > @@ -1428,6 +1428,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-09-25 14:06:58.535276527 +0200
> > +++ gcc/c-family/c-ubsan.c	2013-09-25 14:07:03.580294566 +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-09-25 14:06:58.538276539 +0200
> > +++ gcc/c-family/c-ubsan.h	2013-09-25 14:07:03.595294628 +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-09-25 14:06:58.542276558 +0200
> > +++ gcc/sanitizer.def	2013-09-25 14:07:03.628294753 +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-09-25 14:06:58.546276575 +0200
> > +++ gcc/flag-types.h	2013-09-25 14:07:03.629294757 +0200
> > @@ -201,7 +201,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-09-25 14:06:58.549276587 +0200
> > +++ gcc/cp/decl.c	2013-09-25 14:07:20.640355737 +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"
> > @@ -8465,6 +8466,24 @@ create_array_type_for_decl (tree name, t
> >    if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
> >      pedwarn (input_location, OPT_Wvla, "array of array of runtime bound");
> >  
> > +  /* 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)
> > +    {
> > +      /* Prevent bogus set-but-not-used warnings: we're definitely using
> > +         the variable.  */
> > +      if (VAR_P (size))
> > +        DECL_READ_P (size) = 1;
> > +      /* Evaluate the array size only once.  */
> > +      size = cp_save_expr (size);
> > +      size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
> > +			  ubsan_instrument_vla (input_location, size),
> > +			  size);
> > +    }
> > +
> >    /* Figure out the index type for the array.  */
> >    if (size)
> >      itype = compute_array_index_type (name, size, tf_warning_or_error);
> > --- gcc/c/c-decl.c.mp	2013-09-25 14:06:58.550276591 +0200
> > +++ gcc/c/c-decl.c	2013-09-25 14:07:03.644294820 +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"
> > @@ -5378,6 +5379,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-09-25 14:08:33.263616709 +0200
> > +++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-09-25 14:07:03.650294845 +0200
> > @@ -0,0 +1,13 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
> > +/* { dg-shouldfail "ubsan" } */
> > +
> > +int
> > +main (void)
> > +{
> > +  int y = -18;
> > +  int a[y];
> > +  return 0;
> > +}
> > +
> > +/* { dg-output "terminate called after throwing an instance" } */
> > --- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-09-25 14:08:25.364588140 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-09-25 14:07:03.650294845 +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-09-25 14:08:23.458581265 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-09-25 14:07:03.651294849 +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-09-25 14:08:27.367595369 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-09-25 14:07:03.652294853 +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-09-25 14:08:21.341573677 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-09-25 14:07:03.652294853 +0200
> > @@ -0,0 +1,48 @@
> > +/* { 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 aa[x][x];
> > +  int aaa[x][x][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 -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 -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 -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-09-25 14:06:58.557276623 +0200
> > +++ gcc/asan.c	2013-09-25 14:07:03.653294857 +0200
> > @@ -2018,6 +2018,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
> 
> 	Marek

	Marek

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

[Joseph S. Myers]

On Tue, 15 Oct 2013, Marek Polacek wrote:

> Ping^2.  Jason, Joseph, are you fine with the C++/C FE changes?

The C changes are fine with me.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Jason Merrill]

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.

> +      /* 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.

Jason

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

[Marek Polacek]

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

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

[Jason Merrill]

On 10/25/2013 12:58 PM, Marek Polacek wrote:
> 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...

Because the above is not valid under the proposed standard C++ VLA 
support; only the leftmost bound can be variable.

I think the right place to handle both ubsan and c++1y VLA checks is in 
compute_array_index_type, in the block where we're calling variable_size.

Jason

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

[Marek Polacek]

On Fri, Oct 25, 2013 at 02:17:48PM -0400, Jason Merrill wrote:
> On 10/25/2013 12:58 PM, Marek Polacek wrote:
> >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...
> 
> Because the above is not valid under the proposed standard C++ VLA
> support; only the leftmost bound can be variable.

I see.
 
> I think the right place to handle both ubsan and c++1y VLA checks is
> in compute_array_index_type, in the block where we're calling
> variable_size.

I'm sorry, you want me to move the c++1y VLA check into
compute_array_index_type, or just do the ubsan instrumentation in
there?  Thanks,

	Marek

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

[Jason Merrill]

On 10/25/2013 03:03 PM, Marek Polacek wrote:
> On Fri, Oct 25, 2013 at 02:17:48PM -0400, Jason Merrill wrote:
>> I think the right place to handle both ubsan and c++1y VLA checks is
>> in compute_array_index_type, in the block where we're calling
>> variable_size.
>
> I'm sorry, you want me to move the c++1y VLA check into
> compute_array_index_type, or just do the ubsan instrumentation in
> there?  Thanks,

Both.

Jason

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

[Marek Polacek]

On Fri, Oct 25, 2013 at 03:04:41PM -0400, Jason Merrill wrote:
> >I'm sorry, you want me to move the c++1y VLA check into
> >compute_array_index_type, or just do the ubsan instrumentation in
> >there?  Thanks,
> 
> Both.

Unfortunately, I'm having quite a lot of trouble with side-effects. :(
For e.g.
int x = 1;
int a[++x];

with the following hunk

--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -8394,6 +8382,18 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t com
              if (found)
                itype = variable_size (fold (newitype));
            }
+
+         if ((flag_sanitize & SANITIZE_VLA)
+             && !processing_template_decl
+             /* From C++1y onwards, we throw an exception on a negative
+                length size of an array; see above  */
+             && cxx_dialect < cxx1y)
+           {
+             tree x = cp_save_expr (size);
+             x = build2 (COMPOUND_EXPR, TREE_TYPE (x),
+                         ubsan_instrument_vla (input_location, x), x);
+             finish_expr_stmt (x);
+           }
        }
       /* Make sure that there was no overflow when creating to a signed
         index type.  (For example, on a 32-bit machine, an array with

we generate

  int x = 1;
  int a[0:(sizetype) SAVE_EXPR <D.2143>];

  <<cleanup_point   int x = 1;>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  if (SAVE_EXPR < ++x> <= 0)
    {   
      __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned long) SAVE_EXPR < ++x>);
    }   
  else
    {   
      0   
    }, (void) SAVE_EXPR < ++x>; >>>>>;
    ssizetype D.2143;
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) (D.2143 = (ssizetype)  ++x + -1) >>>>>;
  <<cleanup_point   int a[0:(sizetype) SAVE_EXPR <D.2143>];>>;

that is, x is incremented twice and that is wrong.

Is it possible to tell "x has already been evaluated, don't evaluate
it again" so that the x isn't incremented in the cleanup_point?

Or, would you, please, have some other advice?  I've been looking into this
for quite some time now, but haven't been able to come up with anything
better than moving the checks back to create_array_type_for_decl, where it
all started ;).

	Marek

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

[Jason Merrill]

On 10/30/2013 10:52 AM, Marek Polacek wrote:
> +         if ((flag_sanitize & SANITIZE_VLA)
> +             && !processing_template_decl

You don't need to check processing_template_decl; the template case was 
already handled above.

> +             tree x = cp_save_expr (size);
> +             x = build2 (COMPOUND_EXPR, TREE_TYPE (x),
> +                         ubsan_instrument_vla (input_location, x), x);
> +             finish_expr_stmt (x);

Saving 'size' here doesn't help since it's already been used above. 
Could you use itype instead of size here?

Jason

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

[Marek Polacek]

On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote:
> On 10/30/2013 10:52 AM, Marek Polacek wrote:
> >+         if ((flag_sanitize & SANITIZE_VLA)
> >+             && !processing_template_decl
> 
> You don't need to check processing_template_decl; the template case
> was already handled above.

Right, removed.
 
> >+             tree x = cp_save_expr (size);
> >+             x = build2 (COMPOUND_EXPR, TREE_TYPE (x),
> >+                         ubsan_instrument_vla (input_location, x), x);
> >+             finish_expr_stmt (x);
> 
> Saving 'size' here doesn't help since it's already been used above.
> Could you use itype instead of size here?

I already experimented with that and I think I can't, since we call
the finish_expr_stmt too soon, which results in:

    int x = 1;
    int a[0:(sizetype) SAVE_EXPR <D.2143>];
  
    <<cleanup_point   int x = 1;>>;
    <<cleanup_point <<< Unknown tree: expr_stmt
    if (SAVE_EXPR <D.2143> <= 0)
      {   
        __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned long) SAVE_EXPR <D.2143>);
      }   
    else
      {   
        0   
      }, (void) SAVE_EXPR <D.2143>; >>>>>;
      ssizetype D.2143;
    <<cleanup_point <<< Unknown tree: expr_stmt
    (void) (D.2143 = (ssizetype)  ++x + -1) >>>>>;

and that ICEs in gimplify_var_or_parm_decl, presumably because the
if (SAVE_EXPR <D.2143> <= 0) { ... } should be emitted *after* that
cleanup_point.  When we generated the C++1y check in cp_finish_decl,
we emitted the check after the cleanup_point, and everything was OK.
I admit I don't understand the cleanup_points very much and I don't
know exactly where they are coming from, because normally I don't see
them coming out of C FE. :)  Thanks.

	Marek

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

[Mike Stump]

On Oct 30, 2013, at 9:15 AM, Marek Polacek <polacek@redhat.com> wrote:
> I admit I don't understand the cleanup_points very much and I don't
> know exactly where they are coming from

So, here is the mental model…  and how it is related to the standard.  C++ mandates that destructors for objects and temporary objects run no sooner than a certain place, and no later than another place.  In the implementation, we choose a single point to run them, and use a cleanup point as the embodiment of when destructors run.  For example:

cleanup (a + cleanup (b - c))

means generate this:

a
b
c
-
dtors for things related to b-c
+
dtors for things related to a+ (b-c)

that's it.  Pretty simple.  Now, cute little details, once you get past the simplicity, would be things like, if you run the cleanups for b-c, at the first dtor line above, do you also run those same things at the lower point?  That answer is no, they only run once.  If one takes an exception out of that region, does the cleanup action run?  That answer is yes.  Lots of other possible questions like this, all with fairly simple, easy to understand answers.  Just ask.

Now, some advanced topics…  So, one thing you discover, if you _add_ a cleanup point into an expression, it will run those actions sooner that they would have run, if you had not.  One cannot meet the requirements of the language standard and just arbitrarily add cleanup points.  However, constructs beyond the language standard, say ({ s1; s2; s3; }) + b;, one discovers that the implementation is free to decide if there is a cleanup point for ({ }) or not.  The language standard places no requirements on such code, and this is why we can decide.

decl cleanups are strongly related to these sorts of cleanups, but lie just outside (enclosing).  I'll note their existence for completeness.  See CLEANUP_STMT for these.

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

[Marek Polacek]

Thanks Mike.

I had a quick look at the CLEANUP_STMT and cp-tree.def says
"A CLEANUP_STMT marks the point at which a declaration is fully
constructed.", while doc says
"Used to represent an action that should take place upon exit from the
enclosing scope.  Typically, these actions are calls to destructors for
local objects."  Huh?  So, how come it e.g. initializes variables, and on
the other hand it should run dtors?  I'm baffled (but it's too late for me
to think clearly ;)).

	Marek

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

[Mike Stump]

On Oct 30, 2013, at 3:15 PM, Marek Polacek <polacek@redhat.com> wrote:
> I had a quick look at the CLEANUP_STMT and cp-tree.def says
> "A CLEANUP_STMT marks the point at which a declaration is fully
> constructed.", while doc says
> "Used to represent an action that should take place upon exit from the
> enclosing scope.  Typically, these actions are calls to destructors for
> local objects."  Huh?  So, how come it e.g. initializes variables, and on
> the other hand it should run dtors?  I'm baffled (but it's too late for me
> to think clearly ;)).

The dtors only run, after the ctors run.  We mark where the ctors finish spot, as the _start_ of the region for which we have to clean up.  Really, the cleanup has nothing to do with ctors.  You can have dtors, without any ctors, or ctors, without any dtors.

{
  decl d;
  s;
}

transforms into:

<-----  start of lifetime of the storage for d
ctor(d)
<-----  start of lifetime of the fully constructed object d
s;
<-----  end of lifetime of fully constructed object d
dtor(d)
<-----  end of the storage of d

CLEANUP_STMT documents when the region protected by the cleanup starts.  One want to describe that region is, the end of the ctors, if any, else after the storage is allocated.  In the above, that is the second <---- spot.

Now, in the trees, the above is decl d; ctors; CLEANUP_STMT (s, dtors, d).

s is the region for which the cleanups are active for.  dtors is the cleanup to perform on transfer out of that region, and d is the decl related to the actions in dtors.

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

[Jason Merrill]

On 10/30/2013 12:15 PM, Marek Polacek wrote:
> On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote:
>> Saving 'size' here doesn't help since it's already been used above.
>> Could you use itype instead of size here?
>
> I already experimented with that and I think I can't, since we call
> the finish_expr_stmt too soon, which results in:
>
>      int x = 1;
>      int a[0:(sizetype) SAVE_EXPR <D.2143>];
>
>      <<cleanup_point   int x = 1;>>;
>      <<cleanup_point <<< Unknown tree: expr_stmt
>      if (SAVE_EXPR <D.2143> <= 0)
>        {
>          __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned long) SAVE_EXPR <D.2143>);
>        }
>      else
>        {
>          0
>        }, (void) SAVE_EXPR <D.2143>; >>>>>;
>        ssizetype D.2143;
>      <<cleanup_point <<< Unknown tree: expr_stmt
>      (void) (D.2143 = (ssizetype)  ++x + -1) >>>>>;

Ah, looks like you're getting an unfortunate interaction with 
stabilize_vla_size, which is replacing the contents of the SAVE_EXPR 
with a reference to a variable that isn't initialized yet.  Perhaps we 
should move the stabilize_vla_size call into compute_array_index_type, too.

> and that ICEs in gimplify_var_or_parm_decl, presumably because the
> if (SAVE_EXPR <D.2143> <= 0) { ... } should be emitted *after* that
> cleanup_point.  When we generated the C++1y check in cp_finish_decl,
> we emitted the check after the cleanup_point, and everything was OK.
> I admit I don't understand the cleanup_points very much and I don't
> know exactly where they are coming from, because normally I don't see
> them coming out of C FE. :)

You can ignore the cleanup_points; they just wrap every full-expression.

Jason

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

[Marek Polacek]

On Wed, Oct 30, 2013 at 03:41:53PM -0700, Mike Stump wrote:
> The dtors only run, after the ctors run.  We mark where the ctors finish spot, as the _start_ of the region for which we have to clean up.  Really, the cleanup has nothing to do with ctors.  You can have dtors, without any ctors, or ctors, without any dtors.
> 
> {
>   decl d;
>   s;
> }
> 
> transforms into:
> 
> <-----  start of lifetime of the storage for d
> ctor(d)
> <-----  start of lifetime of the fully constructed object d
> s;
> <-----  end of lifetime of fully constructed object d
> dtor(d)
> <-----  end of the storage of d
> 
> CLEANUP_STMT documents when the region protected by the cleanup starts.  One want to describe that region is, the end of the ctors, if any, else after the storage is allocated.  In the above, that is the second <---- spot.
> 
> Now, in the trees, the above is decl d; ctors; CLEANUP_STMT (s, dtors, d).
> 
> s is the region for which the cleanups are active for.  dtors is the cleanup to perform on transfer out of that region, and d is the decl related to the actions in dtors.

I see now.  Thanks very much, Mike.

	Marek

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

[Marek Polacek]

On Wed, Oct 30, 2013 at 09:10:30PM -0400, Jason Merrill wrote:
> On 10/30/2013 12:15 PM, Marek Polacek wrote:
> >On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote:
> >>Saving 'size' here doesn't help since it's already been used above.
> >>Could you use itype instead of size here?
> >
> >I already experimented with that and I think I can't, since we call
> >the finish_expr_stmt too soon, which results in:
> >
> >     int x = 1;
> >     int a[0:(sizetype) SAVE_EXPR <D.2143>];
> >
> >     <<cleanup_point   int x = 1;>>;
> >     <<cleanup_point <<< Unknown tree: expr_stmt
> >     if (SAVE_EXPR <D.2143> <= 0)
> >       {
> >         __builtin___ubsan_handle_vla_bound_not_positive (&*.Lubsan_data0, (unsigned long) SAVE_EXPR <D.2143>);
> >       }
> >     else
> >       {
> >         0
> >       }, (void) SAVE_EXPR <D.2143>; >>>>>;
> >       ssizetype D.2143;
> >     <<cleanup_point <<< Unknown tree: expr_stmt
> >     (void) (D.2143 = (ssizetype)  ++x + -1) >>>>>;
> 
> Ah, looks like you're getting an unfortunate interaction with
> stabilize_vla_size, which is replacing the contents of the SAVE_EXPR
> with a reference to a variable that isn't initialized yet.  Perhaps
> we should move the stabilize_vla_size call into
> compute_array_index_type, too.

That works, thanks.  So implemented as below in the patch.  I was quite
nervous about dropping the guards before stabilize_vla_size, but
we can't really use them in compute_array_index_type, also I don't
see any new regressions or bootstrap failures.
(The C++1y check was adjusted accordingly to your recent patch, so
we never throw on zero-length arrays.)

Regtested, ran bootstrap-ubsan on x86_64-linux, ok for you now?

2013-10-31  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): Move C++1y bounds checking...
	(compute_array_index_type): ...here.  Add VLA instrumentation.
	Call stabilize_vla_size.
	(grokdeclarator): Don't call stabilize_vla_size here.
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-31 18:06:23.269355759 +0100
+++ gcc/opts.c	2013-10-31 18:06:47.325449575 +0100
@@ -1444,6 +1444,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-31 18:06:23.263355735 +0100
+++ gcc/c-family/c-ubsan.c	2013-10-31 18:06:47.295449445 +0100
@@ -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-31 18:06:23.264355739 +0100
+++ gcc/c-family/c-ubsan.h	2013-10-31 18:06:47.296449449 +0100
@@ -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-31 18:06:23.270355763 +0100
+++ gcc/sanitizer.def	2013-10-31 18:06:47.327449583 +0100
@@ -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-31 18:06:23.268355755 +0100
+++ gcc/flag-types.h	2013-10-31 18:06:47.324449570 +0100
@@ -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-31 18:06:23.267355751 +0100
+++ gcc/cp/decl.c	2013-10-31 18:06:47.320449552 +0100
@@ -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,17 +6400,6 @@ 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 (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
-    {
-      /* If the VLA bound is larger than half the address space, or less
-	 than zero, throw std::bad_array_length.  */
-      tree max = convert (ssizetype, TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
-      tree comp = build2 (LT_EXPR, boolean_type_node, max, ssize_int (-1));
-      comp = build3 (COND_EXPR, void_type_node, comp,
-		     throw_bad_array_length (), void_zero_node);
-      finish_expr_stmt (comp);
-    }
-
   /* Add this declaration to the statement-tree.  This needs to happen
      after the call to check_initializer so that the DECL_EXPR for a
      reference temp is added before the DECL_EXPR for the reference itself.  */
@@ -8379,6 +8369,11 @@ compute_array_index_type (tree name, tre
 	{
 	  /* A variable sized array.  */
 	  itype = variable_size (itype);
+
+	  /* We need to stabilize side-effects in VLA sizes for regular array
+	     declarations too, not just pointers to arrays.  */
+	  stabilize_vla_size (itype);
+
 	  if (TREE_CODE (itype) != SAVE_EXPR)
 	    {
 	      /* Look for SIZEOF_EXPRs in itype and fold them, otherwise
@@ -8390,6 +8385,31 @@ compute_array_index_type (tree name, tre
 	      if (found)
 		itype = variable_size (fold (newitype));
 	    }
+
+	  if (cxx_dialect >= cxx1y)
+	    {
+	      /* If the VLA bound is larger than half the address space,
+	         or less than zero, throw std::bad_array_length.  */
+	      tree comp = build2 (LT_EXPR, boolean_type_node, itype,
+				  ssize_int (-1));
+	      comp = build3 (COND_EXPR, void_type_node, comp,
+			     throw_bad_array_length (), void_zero_node);
+	      finish_expr_stmt (comp);
+	  }
+
+         if ((flag_sanitize & SANITIZE_VLA)
+             /* From C++1y onwards, we throw an exception on a negative
+                length size of an array; see above  */
+             && cxx_dialect < cxx1y)
+           {
+	     /* We have to add 1 -- in the ubsan routine we generate
+	        LE_EXPR rather than LT_EXPR.  */
+	     tree t = fold_build2 (PLUS_EXPR, TREE_TYPE (itype), itype,
+				   build_one_cst (TREE_TYPE (itype)));
+             t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t),
+			      ubsan_instrument_vla (input_location, t), t);
+             finish_expr_stmt (t);
+           }
 	}
       /* Make sure that there was no overflow when creating to a signed
 	 index type.  (For example, on a 32-bit machine, an array with
@@ -9886,14 +9906,6 @@ grokdeclarator (const cp_declarator *dec
 	}
     }
 
-  /* We need to stabilize side-effects in VLA sizes for regular array
-     declarations too, not just pointers to arrays.  */
-  if (type != error_mark_node && !TYPE_NAME (type)
-      && (decl_context == NORMAL || decl_context == FIELD)
-      && at_function_scope_p ()
-      && variably_modified_type_p (type, NULL_TREE))
-    stabilize_vla_size (TYPE_SIZE (type));
-
   /* A `constexpr' specifier used in an object declaration declares
      the object as `const'.  */
   if (constexpr_p && innermost_code != cdk_function)
--- gcc/c/c-decl.c.mp	2013-10-31 18:06:23.265355743 +0100
+++ gcc/c/c-decl.c	2013-10-31 18:06:47.308449500 +0100
@@ -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"
@@ -5411,6 +5412,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-10-31 18:09:03.019981937 +0100
+++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-10-31 18:08:42.117900326 +0100
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
+/* { dg-shouldfail "ubsan" } */
+
+int
+main (void)
+{
+  int y = -18;
+  int a[y];
+  return 0;
+}
+
+/* { dg-output "terminate called after throwing an instance" } */
--- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-10-31 18:08:49.639929788 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-10-31 18:08:42.117900326 +0100
@@ -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-31 18:08:53.347944329 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-10-31 18:08:42.118900330 +0100
@@ -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-31 18:08:56.852958019 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-10-31 18:08:42.119900334 +0100
@@ -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-31 18:08:51.771938127 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-10-31 18:08:42.119900334 +0100
@@ -0,0 +1,48 @@
+/* { 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 aa[x][x];
+  int aaa[x][x][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 -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 -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 -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-31 18:06:02.861276291 +0100
+++ gcc/asan.c	2013-10-31 18:06:47.293449437 +0100
@@ -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

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

[Jason Merrill]

On 10/31/2013 02:28 PM, Marek Polacek wrote:
>   	  /* A variable sized array.  */
>   	  itype = variable_size (itype);
> +
> +	  /* We need to stabilize side-effects in VLA sizes for regular array
> +	     declarations too, not just pointers to arrays.  */
> +	  stabilize_vla_size (itype);

Let's put this after the later call to variable_size, too.

>   	  if (TREE_CODE (itype) != SAVE_EXPR)
>   	    {
>   	      /* Look for SIZEOF_EXPRs in itype and fold them, otherwise
> @@ -8390,6 +8385,31 @@ compute_array_index_type (tree name, tre
>   	      if (found)
>   		itype = variable_size (fold (newitype));
>   	    }

i.e. here.

> +
> +	  if (cxx_dialect >= cxx1y)
> +	    {
> +	      /* If the VLA bound is larger than half the address space,
> +	         or less than zero, throw std::bad_array_length.  */
> +	      tree comp = build2 (LT_EXPR, boolean_type_node, itype,
> +				  ssize_int (-1));
> +	      comp = build3 (COND_EXPR, void_type_node, comp,
> +			     throw_bad_array_length (), void_zero_node);
> +	      finish_expr_stmt (comp);
> +	  }
> +
> +         if ((flag_sanitize & SANITIZE_VLA)
> +             /* From C++1y onwards, we throw an exception on a negative
> +                length size of an array; see above  */
> +             && cxx_dialect < cxx1y)

This could be

   else if (flag_sanitize & SANITIZE_VLA)

There's another use of stabilize_vla_size in grokdeclarator, that should 
be able to go as well.

Jason

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

[Marek Polacek]

On Fri, Nov 01, 2013 at 01:35:07PM -0400, Jason Merrill wrote:
> On 10/31/2013 02:28 PM, Marek Polacek wrote:
> >  	  /* A variable sized array.  */
> >  	  itype = variable_size (itype);
> >+
> >+	  /* We need to stabilize side-effects in VLA sizes for regular array
> >+	     declarations too, not just pointers to arrays.  */
> >+	  stabilize_vla_size (itype);
> 
> Let's put this after the later call to variable_size, too.
> 
> >  	  if (TREE_CODE (itype) != SAVE_EXPR)
> >  	    {
> >  	      /* Look for SIZEOF_EXPRs in itype and fold them, otherwise
> >@@ -8390,6 +8385,31 @@ compute_array_index_type (tree name, tre
> >  	      if (found)
> >  		itype = variable_size (fold (newitype));
> >  	    }
> 
> i.e. here.

Done.
 
> >+
> >+	  if (cxx_dialect >= cxx1y)
> >+	    {
> >+	      /* If the VLA bound is larger than half the address space,
> >+	         or less than zero, throw std::bad_array_length.  */
> >+	      tree comp = build2 (LT_EXPR, boolean_type_node, itype,
> >+				  ssize_int (-1));
> >+	      comp = build3 (COND_EXPR, void_type_node, comp,
> >+			     throw_bad_array_length (), void_zero_node);
> >+	      finish_expr_stmt (comp);
> >+	  }
> >+
> >+         if ((flag_sanitize & SANITIZE_VLA)
> >+             /* From C++1y onwards, we throw an exception on a negative
> >+                length size of an array; see above  */
> >+             && cxx_dialect < cxx1y)
> 
> This could be
> 
>   else if (flag_sanitize & SANITIZE_VLA)

Done (with some formatting nits fixed).

> There's another use of stabilize_vla_size in grokdeclarator, that
> should be able to go as well.

Yep, seems so.  

Regtest/bootstrap on x86_64-linux running (so far lookin' good), ok if passes?

2013-11-01  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): Move C++1y bounds checking...
	(compute_array_index_type): ...here.  Add VLA instrumentation.
	Call stabilize_vla_size.
	(grokdeclarator): Don't call stabilize_vla_size here.
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-11-01 18:53:17.372014692 +0100
+++ gcc/opts.c	2013-11-01 18:53:48.857130430 +0100
@@ -1444,6 +1444,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-11-01 18:53:17.389014762 +0100
+++ gcc/c-family/c-ubsan.c	2013-11-01 18:53:48.858130434 +0100
@@ -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-11-01 18:53:17.395014787 +0100
+++ gcc/c-family/c-ubsan.h	2013-11-01 18:53:48.864130461 +0100
@@ -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-11-01 18:53:17.403014823 +0100
+++ gcc/sanitizer.def	2013-11-01 18:53:48.865130465 +0100
@@ -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-11-01 18:53:17.405014831 +0100
+++ gcc/flag-types.h	2013-11-01 18:53:48.866130469 +0100
@@ -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-11-01 18:53:17.425014915 +0100
+++ gcc/cp/decl.c	2013-11-01 19:44:46.897799049 +0100
@@ -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,17 +6400,6 @@ 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 (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type))
-    {
-      /* If the VLA bound is larger than half the address space, or less
-	 than zero, throw std::bad_array_length.  */
-      tree max = convert (ssizetype, TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
-      tree comp = build2 (LT_EXPR, boolean_type_node, max, ssize_int (-1));
-      comp = build3 (COND_EXPR, void_type_node, comp,
-		     throw_bad_array_length (), void_zero_node);
-      finish_expr_stmt (comp);
-    }
-
   /* Add this declaration to the statement-tree.  This needs to happen
      after the call to check_initializer so that the DECL_EXPR for a
      reference temp is added before the DECL_EXPR for the reference itself.  */
@@ -8379,6 +8369,7 @@ compute_array_index_type (tree name, tre
 	{
 	  /* A variable sized array.  */
 	  itype = variable_size (itype);
+
 	  if (TREE_CODE (itype) != SAVE_EXPR)
 	    {
 	      /* Look for SIZEOF_EXPRs in itype and fold them, otherwise
@@ -8390,6 +8381,34 @@ compute_array_index_type (tree name, tre
 	      if (found)
 		itype = variable_size (fold (newitype));
 	    }
+
+	  /* We need to stabilize side-effects in VLA sizes for regular array
+	     declarations too, not just pointers to arrays.  */
+	  stabilize_vla_size (itype);
+
+	  if (cxx_dialect >= cxx1y)
+	    {
+	      /* If the VLA bound is larger than half the address space,
+	         or less than zero, throw std::bad_array_length.  */
+	      tree comp = build2 (LT_EXPR, boolean_type_node, itype,
+				  ssize_int (-1));
+	      comp = build3 (COND_EXPR, void_type_node, comp,
+			     throw_bad_array_length (), void_zero_node);
+	      finish_expr_stmt (comp);
+	    }
+	  else if (flag_sanitize & SANITIZE_VLA)
+	    {
+	      /* From C++1y onwards, we throw an exception on a negative
+		 length size of an array; see above.  */
+
+	      /* We have to add 1 -- in the ubsan routine we generate
+		 LE_EXPR rather than LT_EXPR.  */
+	      tree t = fold_build2 (PLUS_EXPR, TREE_TYPE (itype), itype,
+				    build_one_cst (TREE_TYPE (itype)));
+	      t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t),
+			       ubsan_instrument_vla (input_location, t), t);
+	      finish_expr_stmt (t);
+	    }
 	}
       /* Make sure that there was no overflow when creating to a signed
 	 index type.  (For example, on a 32-bit machine, an array with
@@ -9790,12 +9809,8 @@ grokdeclarator (const cp_declarator *dec
 	      && (decl_context == NORMAL || decl_context == FIELD)
 	      && at_function_scope_p ()
 	      && variably_modified_type_p (type, NULL_TREE))
-	    {
-	      /* First break out any side-effects.  */
-	      stabilize_vla_size (TYPE_SIZE (type));
-	      /* And then force evaluation of the SAVE_EXPR.  */
-	      finish_expr_stmt (TYPE_SIZE (type));
-	    }
+	    /* Force evaluation of the SAVE_EXPR.  */
+	    finish_expr_stmt (TYPE_SIZE (type));
 
 	  if (declarator->kind == cdk_reference)
 	    {
@@ -9886,14 +9901,6 @@ grokdeclarator (const cp_declarator *dec
 	}
     }
 
-  /* We need to stabilize side-effects in VLA sizes for regular array
-     declarations too, not just pointers to arrays.  */
-  if (type != error_mark_node && !TYPE_NAME (type)
-      && (decl_context == NORMAL || decl_context == FIELD)
-      && at_function_scope_p ()
-      && variably_modified_type_p (type, NULL_TREE))
-    stabilize_vla_size (TYPE_SIZE (type));
-
   /* A `constexpr' specifier used in an object declaration declares
      the object as `const'.  */
   if (constexpr_p && innermost_code != cdk_function)
--- gcc/c/c-decl.c.mp	2013-11-01 18:53:17.443014991 +0100
+++ gcc/c/c-decl.c	2013-11-01 18:53:48.946130794 +0100
@@ -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"
@@ -5411,6 +5412,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/g++.dg/ubsan/cxx1y-vla.C.mp	2013-11-01 18:53:55.957155884 +0100
+++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C	2013-11-01 18:53:48.956130834 +0100
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */
+/* { dg-shouldfail "ubsan" } */
+
+int
+main (void)
+{
+  int y = -18;
+  int a[y];
+  return 0;
+}
+
+/* { dg-output "terminate called after throwing an instance" } */
--- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp	2013-11-01 18:54:05.024188578 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-3.c	2013-11-01 18:53:48.957130838 +0100
@@ -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-11-01 18:54:08.840202633 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-2.c	2013-11-01 18:53:48.957130838 +0100
@@ -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-11-01 18:54:10.576209008 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-4.c	2013-11-01 18:53:48.957130838 +0100
@@ -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-11-01 18:54:07.233196627 +0100
+++ gcc/testsuite/c-c++-common/ubsan/vla-1.c	2013-11-01 18:53:48.958130842 +0100
@@ -0,0 +1,48 @@
+/* { 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 aa[x][x];
+  int aaa[x][x][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 -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 -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 -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-11-01 18:53:17.445015000 +0100
+++ gcc/asan.c	2013-11-01 18:53:48.959130846 +0100
@@ -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

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

[Jason Merrill]

On 11/01/2013 03:10 PM, Marek Polacek wrote:
> +	  /* We need to stabilize side-effects in VLA sizes for regular array
> +	     declarations too, not just pointers to arrays.  */

This comment isn't really relevant to its new location.  :)

OK with that removed.

Jason

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

[Marek Polacek]

On Fri, Nov 01, 2013 at 04:39:09PM -0400, Jason Merrill wrote:
> On 11/01/2013 03:10 PM, Marek Polacek wrote:
> >+	  /* We need to stabilize side-effects in VLA sizes for regular array
> >+	     declarations too, not just pointers to arrays.  */
> 
> This comment isn't really relevant to its new location.  :)
> 
> OK with that removed.

Sure, thanks.  Passed various testing, will install this soon.

	Marek