This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix PR c++/19351 (operator new[] overflow)
- From: Florian Weimer <fweimer at redhat dot com>
- To: Jason Merrill <jason at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Fri, 10 Aug 2012 17:02:43 +0200
- Subject: Re: Fix PR c++/19351 (operator new[] overflow)
- References: <4FD9B4F6.4030200@redhat.com> <4FE9C75A.8030000@redhat.com> <5006C035.8030100@redhat.com> <5006C8A8.1080309@redhat.com>
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.
--
Florian Weimer / Red Hat Product Security Team
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;
+}
+