diff mbox

Fix PR c++/19351 (operator new[] overflow)

Message ID 4FE9C75A.8030000@redhat.com
State New
Headers show

Commit Message

Florian Weimer June 26, 2012, 2:29 p.m. UTC
On 06/14/2012 11:55 AM, Florian Weimer wrote:
> This is another attempt at ensuring that operator new[] always returns a
> block of sufficient size.
>
> This is on top of my previous patch rejecting VLA allocations:
> http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00616.html

I've committed the patch referenced above, and rebased this patch.  I 
also made two additional changes: The Hamming weight reduction (to help 
encoding the maximum size constant on architectures like ARM) was 
unnecessarily aggressive, and I had forgotten to disable cookies for 
non-cookie types, leading to failures when testing Java.

Bootstrapped and tested on x86_86-unknown-linux-gnu, with no new 
regressions (this time including Java).  Okay for trunk?

Comments

Florian Weimer July 17, 2012, 9:01 a.m. UTC | #1
On 06/26/2012 04:29 PM, Florian Weimer wrote:

> Bootstrapped and tested on x86_86-unknown-linux-gnu, with no new
> regressions (this time including Java).  Okay for trunk?

Ping?
Jason Merrill July 18, 2012, 1:55 p.m. UTC | #2
On 06/26/2012 10:29 AM, Florian Weimer wrote:
> +  /* Set to (size_t)-1 if the size check fails.  */
> +  if (size_check != NULL_TREE)
> +    *size = fold_build3 (COND_EXPR, sizetype, size_check,
> +			 original_size, TYPE_MAX_VALUE (sizetype));
>     VEC_safe_insert (tree, gc, *args, 0, *size);
>     *args = resolve_args (*args, complain);
>     if (*args == NULL)
> @@ -4022,7 +4030,11 @@ build_operator_new_call (tree fnname, VEC(tree,gc) **args,
>          if (use_cookie)
>   	 {
>   	   /* Update the total size.  */
> -	   *size = size_binop (PLUS_EXPR, *size, *cookie_size);
> +	   *size = size_binop (PLUS_EXPR, original_size, *cookie_size);
> +	   /* Set to (size_t)-1 if the size check fails.  */
> +	   gcc_assert (size_check != NULL_TREE);
> +	   *size = fold_build3 (COND_EXPR, sizetype, size_check,
> +				*size, TYPE_MAX_VALUE (sizetype));

Looks like you're evaluating the size_check twice for types that use 
cookies.

> +      /* Unconditionally substract the array size.  This decreases the
> +	 maximum object size and is safe even if we choose not to use
> +	 a cookie after all.  */

"cookie size"

But since we're going to be deciding whether or not to use a cookie in 
this function anyway, why not do it here?

Jason
Florian Weimer July 18, 2012, 2:31 p.m. UTC | #3
On 07/18/2012 03:55 PM, Jason Merrill wrote:
> On 06/26/2012 10:29 AM, Florian Weimer wrote:
>> +  /* Set to (size_t)-1 if the size check fails.  */
>> +  if (size_check != NULL_TREE)
>> +    *size = fold_build3 (COND_EXPR, sizetype, size_check,
>> +             original_size, TYPE_MAX_VALUE (sizetype));
>>     VEC_safe_insert (tree, gc, *args, 0, *size);
>>     *args = resolve_args (*args, complain);
>>     if (*args == NULL)
>> @@ -4022,7 +4030,11 @@ build_operator_new_call (tree fnname,
>> VEC(tree,gc) **args,
>>          if (use_cookie)
>>        {
>>          /* Update the total size.  */
>> -       *size = size_binop (PLUS_EXPR, *size, *cookie_size);
>> +       *size = size_binop (PLUS_EXPR, original_size, *cookie_size);
>> +       /* Set to (size_t)-1 if the size check fails.  */
>> +       gcc_assert (size_check != NULL_TREE);
>> +       *size = fold_build3 (COND_EXPR, sizetype, size_check,
>> +                *size, TYPE_MAX_VALUE (sizetype));
>
> Looks like you're evaluating the size_check twice for types that use
> cookies.

I try to avoid this by using original_size instead of size on the first 
assignment under the use_cookie case.

>> +      /* Unconditionally substract the array size.  This decreases the
>> +     maximum object size and is safe even if we choose not to use
>> +     a cookie after all.  */
>
> "cookie size"

Okay, I will fix that.

> But since we're going to be deciding whether or not to use a cookie in
> this function anyway, why not do it here?

The decision to use a cookie is currently split between the two 
functions and there are several special cases (Java, ABI compatibility 
flags).  I did not want to disturb this code too much because we do not 
have much test suite coverage in this area.
diff mbox

Patch

2012-06-26  Florian Weimer  <fweimer@redhat.com>

	PR c++/19351
	* call.c (build_operator_new_call): Add size_check argument and
	evaluate it.
	* cp-tree.h (build_operator_new_call): Adjust declaration.
	* init.c (build_new_1): Compute array size check and apply it.

2012-06-26  Florian Weimer  <fweimer@redhat.com>

	* g++.dg/init/new38.C: New test.
	* g++.dg/init/new39.C: New test.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 09965b3..806e0ba 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -3943,15 +3943,19 @@  build_new_function_call (tree fn, VEC(tree,gc) **args, bool koenig_p,
    total number of bytes required by the allocation, and is updated if
    that is changed here.  *COOKIE_SIZE is non-NULL if a cookie should
    be used.  If this function determines that no cookie should be
-   used, after all, *COOKIE_SIZE is set to NULL_TREE.  If FN is
-   non-NULL, it will be set, upon return, to the allocation function
-   called.  */
+   used, after all, *COOKIE_SIZE is set to NULL_TREE.  If SIZE_CHECK
+   is not NULL_TREE, it is evaluated before calculating the final
+   array size, and if it fails, the array size is replaced with
+   (size_t)-1 (usually triggering a std::bad_alloc exception).  If FN
+   is non-NULL, it will be set, upon return, to the allocation
+   function called.  */
 
 tree
 build_operator_new_call (tree fnname, VEC(tree,gc) **args,
-			 tree *size, tree *cookie_size,
+			 tree *size, tree *cookie_size, tree size_check,
 			 tree *fn, tsubst_flags_t complain)
 {
+  tree original_size = *size;
   tree fns;
   struct z_candidate *candidates;
   struct z_candidate *cand;
@@ -3959,6 +3963,10 @@  build_operator_new_call (tree fnname, VEC(tree,gc) **args,
 
   if (fn)
     *fn = NULL_TREE;
+  /* Set to (size_t)-1 if the size check fails.  */
+  if (size_check != NULL_TREE)
+    *size = fold_build3 (COND_EXPR, sizetype, size_check,
+			 original_size, TYPE_MAX_VALUE (sizetype));
   VEC_safe_insert (tree, gc, *args, 0, *size);
   *args = resolve_args (*args, complain);
   if (*args == NULL)
@@ -4022,7 +4030,11 @@  build_operator_new_call (tree fnname, VEC(tree,gc) **args,
        if (use_cookie)
 	 {
 	   /* Update the total size.  */
-	   *size = size_binop (PLUS_EXPR, *size, *cookie_size);
+	   *size = size_binop (PLUS_EXPR, original_size, *cookie_size);
+	   /* Set to (size_t)-1 if the size check fails.  */
+	   gcc_assert (size_check != NULL_TREE);
+	   *size = fold_build3 (COND_EXPR, sizetype, size_check,
+				*size, TYPE_MAX_VALUE (sizetype));
 	   /* Update the argument list to reflect the adjusted size.  */
 	   VEC_replace (tree, *args, 0, *size);
 	 }
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index a4b7ae3..9033108 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4879,7 +4879,7 @@  extern tree build_user_type_conversion		(tree, tree, int,
 extern tree build_new_function_call		(tree, VEC(tree,gc) **, bool, 
 						 tsubst_flags_t);
 extern tree build_operator_new_call		(tree, VEC(tree,gc) **, tree *,
-						 tree *, tree *,
+						 tree *, tree, tree *,
 						 tsubst_flags_t);
 extern tree build_new_method_call		(tree, tree, VEC(tree,gc) **,
 						 tree, int, tree *,
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 5a81643..2f2ef69 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2175,7 +2175,10 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
   tree pointer_type;
   tree non_const_pointer_type;
   tree outer_nelts = NULL_TREE;
+  /* For arrays, a bounds checks on the NELTS parameter. */
+  tree outer_nelts_check = NULL_TREE;
   bool outer_nelts_from_type = false;
+  double_int inner_nelts_count = double_int_one;
   tree alloc_call, alloc_expr;
   /* The address returned by the call to "operator new".  This node is
      a VAR_DECL and is therefore reusable.  */
@@ -2228,7 +2231,22 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
     {
       tree inner_nelts = array_type_nelts_top (elt_type);
       tree inner_nelts_cst = maybe_constant_value (inner_nelts);
-      if (!TREE_CONSTANT (inner_nelts_cst))
+      if (TREE_CONSTANT (inner_nelts_cst)
+	  && TREE_CODE (inner_nelts_cst) == INTEGER_CST)
+	{
+	  double_int result;
+	  if (mul_double (TREE_INT_CST_LOW (inner_nelts_cst),
+			  TREE_INT_CST_HIGH (inner_nelts_cst),
+			  inner_nelts_count.low, inner_nelts_count.high,
+			  &result.low, &result.high))
+	    {
+	      if (complain & tf_error)
+		error ("integer overflow in array size");
+	      nelts = error_mark_node;
+	    }
+	  inner_nelts_count = result;
+	}
+      else
 	{
 	  if (complain & tf_error)
 	    {
@@ -2318,7 +2336,56 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
 
   size = size_in_bytes (elt_type);
   if (array_p)
-    size = size_binop (MULT_EXPR, size, convert (sizetype, nelts));
+    {
+      /* Maximum available size in bytes.  Half of the address space
+	 minus the cookie size.  */
+      double_int max_size
+	= double_int_lshift (double_int_one, TYPE_PRECISION (sizetype) - 1,
+			     HOST_BITS_PER_DOUBLE_INT, false);
+      /* Size of the inner array elements. */
+      double_int inner_size;
+      /* Maximum number of outer elements which can be allocated. */
+      double_int max_outer_nelts;
+      tree max_outer_nelts_tree;
+
+      gcc_assert (TREE_CODE (size) == INTEGER_CST);
+      cookie_size = targetm.cxx.get_cookie_size (elt_type);
+      gcc_assert (TREE_CODE (cookie_size) == INTEGER_CST);
+      gcc_checking_assert (double_int_ucmp
+			   (TREE_INT_CST (cookie_size), max_size) < 0);
+      /* Unconditionally substract the array size.  This decreases the
+	 maximum object size and is safe even if we choose not to use
+	 a cookie after all.  */
+      max_size = double_int_sub (max_size, TREE_INT_CST (cookie_size));
+      if (mul_double (TREE_INT_CST_LOW (size), TREE_INT_CST_HIGH (size),
+		      inner_nelts_count.low, inner_nelts_count.high,
+		      &inner_size.low, &inner_size.high)
+	  || double_int_ucmp (inner_size, max_size) > 0)
+	{
+	  if (complain & tf_error)
+	    error ("size of array is too large");
+	  return error_mark_node;
+	}
+      max_outer_nelts = double_int_udiv (max_size, inner_size, TRUNC_DIV_EXPR);
+      /* Only keep the top-most seven bits, to simplify encoding the
+	 constant in the instruction stream.  */
+      {
+	unsigned shift = HOST_BITS_PER_DOUBLE_INT - 7
+	  - (max_outer_nelts.high ? clz_hwi (max_outer_nelts.high)
+	     : (HOST_BITS_PER_WIDE_INT + clz_hwi (max_outer_nelts.low)));
+	max_outer_nelts
+	  = double_int_lshift (double_int_rshift
+			       (max_outer_nelts, shift,
+				HOST_BITS_PER_DOUBLE_INT, false),
+			       shift, HOST_BITS_PER_DOUBLE_INT, false);
+      }
+      max_outer_nelts_tree = double_int_to_tree (sizetype, max_outer_nelts);
+
+      size = size_binop (MULT_EXPR, size, convert (sizetype, nelts));
+      outer_nelts_check = fold_build2 (LE_EXPR, boolean_type_node,
+				       outer_nelts,
+				       max_outer_nelts_tree);
+    }
 
   alloc_fn = NULL_TREE;
 
@@ -2381,10 +2448,13 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
 	  /* Use a class-specific operator new.  */
 	  /* If a cookie is required, add some extra space.  */
 	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
-	    {
-	      cookie_size = targetm.cxx.get_cookie_size (elt_type);
-	      size = size_binop (PLUS_EXPR, size, cookie_size);
-	    }
+	    size = size_binop (PLUS_EXPR, size, cookie_size);
+	  else
+	    cookie_size = NULL_TREE;
+	  /* Perform the overflow check.  */
+	  if (outer_nelts_check != NULL_TREE)
+            size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
+                                size, TYPE_MAX_VALUE (sizetype));
 	  /* Create the argument list.  */
 	  VEC_safe_insert (tree, gc, *placement, 0, size);
 	  /* Do name-lookup to find the appropriate operator.  */
@@ -2415,13 +2485,12 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
 	{
 	  /* Use a global operator new.  */
 	  /* See if a cookie might be required.  */
-	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
-	    cookie_size = targetm.cxx.get_cookie_size (elt_type);
-	  else
+	  if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)))
 	    cookie_size = NULL_TREE;
 
 	  alloc_call = build_operator_new_call (fnname, placement,
 						&size, &cookie_size,
+						outer_nelts_check,
 						&alloc_fn, complain);
 	}
     }
diff --git a/gcc/testsuite/g++.dg/init/new38.C b/gcc/testsuite/g++.dg/init/new38.C
new file mode 100644
index 0000000..1672f22
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new38.C
@@ -0,0 +1,54 @@ 
+// { dg-do compile }
+
+void
+large_array_char(int n)
+{
+  new char[n]
+    [1ULL << (sizeof(void *) * 4)]
+    [1ULL << (sizeof(void *) * 4)]; // { dg-error "size of array" }
+}
+
+template <typename T>
+void
+large_array_char_template(int n)
+{
+  new char[n]
+    [1ULL << (sizeof(void *) * 4)]
+    [1ULL << (sizeof(void *) * 4)]; // { dg-error "size of array" }
+}
+
+
+template <typename T>
+void
+large_array_template1(int n)
+{
+  new T[n] // { dg-error "size of array is too large" }
+    [(1ULL << (sizeof(void *) * 4)) / sizeof(T)]
+    [1ULL << (sizeof(void *) * 4)];
+}
+
+template <typename T>
+void
+large_array_template2(int n)
+{
+  new T[n] // { dg-error "size of array is too large" }
+    [(1ULL << (sizeof(void *) * 4)) / sizeof(T)]
+    [1ULL << (sizeof(void *) * 4)];
+}
+
+template <typename T>
+void
+large_array_template3(int n)
+{
+  new T[n] // { dg-error "size of array is too large" }
+    [(1ULL << (sizeof(void *) * 4)) / sizeof(T)]
+    [1ULL << (sizeof(void *) * 4)];
+}
+
+void
+call_large_array_template(int n)
+{
+  large_array_template1<char>(n);
+  large_array_template2<int>(n);
+  large_array_template3<double>(n);
+}
diff --git a/gcc/testsuite/g++.dg/init/new39.C b/gcc/testsuite/g++.dg/init/new39.C
new file mode 100644
index 0000000..f274ebb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new39.C
@@ -0,0 +1,68 @@ 
+// Testcase for overflow handling in operator new[].
+// { dg-do run }
+
+#include <stdlib.h>
+#include <stdexcept>
+
+struct without_new {
+  char bar[256];
+};
+
+struct with_new {
+  char bar[256];
+  void *operator new[] (size_t sz)
+  {
+    if (sz != -1)
+      abort ();
+    throw std::bad_alloc();
+  }
+};
+
+template <typename T>
+inline void
+test (size_t s)
+{
+  try {
+    new T[s];
+    abort ();
+  } catch (std::bad_alloc &) {
+  }
+}
+
+template <typename T>
+void
+test_noopt (size_t s) __attribute__((noinline));
+
+template <typename T>
+void
+test_noopt (size_t s)
+{
+  __asm__ ("");
+  test<T> (s);
+}
+
+template <typename T>
+void
+all_tests ()
+{
+  test<T>(-1);
+  test<T>(size_t(-1) / sizeof (T) + 1);
+  test<T>(size_t(-1) / sizeof (T) + 2);
+  test_noopt<T>(-1);
+  test_noopt<T>(size_t(-1) / sizeof (T) + 1);
+  test_noopt<T>(size_t(-1) / sizeof (T) + 2);
+}
+
+int
+main ()
+{
+  try {
+    ::operator new(size_t(-1));
+    abort ();
+  } catch (std::bad_alloc &) {
+  }
+  all_tests<without_new> ();
+  all_tests<with_new> ();
+  return 0;
+}
+