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

login
register
mail settings
Submitter Florian Weimer
Date Aug. 10, 2012, 3:02 p.m.
Message ID <50252293.6090400@redhat.com>
Download mbox | patch
Permalink /patch/176508/
State New
Headers show

Comments

Florian Weimer - Aug. 10, 2012, 3:02 p.m.
On 07/18/2012 04:31 PM, Florian Weimer wrote:
> 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.

Ping?

I'm attaching the current patch.
Jason Merrill - Aug. 20, 2012, 8:24 p.m.
OK.  Sorry for the delay.

Jason

Patch

2012-08-10  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-08-10  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 5345f2b..46a4bcb 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -3941,15 +3941,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;
@@ -3957,6 +3961,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)
@@ -4020,7 +4028,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 44f3ac1..f3dccf9 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4886,7 +4886,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 a725a0c..09288f8 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2178,7 +2178,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.  */
@@ -2231,7 +2234,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)
 	    {
@@ -2321,7 +2339,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 cookie 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;
 
@@ -2384,10 +2451,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.  */
@@ -2418,13 +2488,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;
+}
+