Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
On 11/26/2015 10:45 AM, Martin Sebor wrote: On 11/26/2015 04:33 AM, Ramana Radhakrishnan wrote: Cookies on ARM are 8-bytes [1], but sizeof ((size_t) n) is only 4-bytes, so this check will fail (We'll ask for 500 bytes, the test here will only be looking for 496). Would it undermine the test for other architectures if I were to swap out the != for a >= ? I think that is in line with the "argument large enough for the array" that this test is looking for, but would not catch bugs where we were allocating more memory than neccessary. Otherwise I can spin a patch which skips the test for ARM targets. I didn't want to skip this for ARM, instead something that takes into account the cookie size - (very gratuitous hack was to just add 4 in a #ifdef __arm__ block). Something like attached, brown paper bag warning ;) Thanks. I'll commit it today after some testing. I've checked in a slightly modified version of your patch in r230987. I should probably also check to see if there are other such targets and try to find a way to generalize the test. (There should be a way to expose the cookie size to programs, otherwise they have no way to avoid buffer overflow in array forms of placement new). There don't appear to be other targets besides ARM that override the default cookie size. I opened enhancement c++/68571 - provide __builtin_cookie_size, to provide an API to make it possible for programs to query this constant. Martin
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
On Thu, Nov 05, 2015 at 12:30:08PM -0700, Martin Sebor wrote:
> On 11/02/2015 09:55 PM, Jason Merrill wrote:
> >On 10/26/2015 10:06 PM, Martin Sebor wrote:
> >>+ if (TREE_CONSTANT (maybe_constant_value (outer_nelts)))
> >>+{
> >>+ if (tree_int_cst_lt (max_outer_nelts_tree, outer_nelts))
> >
> >maybe_constant_value may return a constant, but that doesn't mean that
> >outer_nelts was already constant; if it wasn't, the call to
> >tree_int_cst_lt will fail.
>
> Thanks for the hint. I wasn't able to trigger the failure. I suspect
> outer_nelts must have already been folded at this point because the
> maybe_constant_value call isn't necessary. I removed it.
>
> >Since we're moving toward delayed folding, I'd prefer to use the result
> >of maybe_constant_value only for this diagnostic, and then continue to
> >pass the unfolded value along.
>
> Sure. Done in the attached patch.
>
> Martin
> gcc/cp/ChangeLog
>
> 2015-10-19 Martin Sebor
>
> PR c++/67913
> PR c++/67927
> * call.c (build_operator_new_call): Do not assume size_check
> is non-null, analogously to the top half of the function.
> * init.c (build_new_1): Detect and diagnose array sizes in
> excess of the maximum of roughly SIZE_MAX / 2.
> Insert a runtime check only for arrays with a non-constant size.
> (build_new): Detect and diagnose negative array sizes.
>
> gcc/testsuite/ChangeLog
>
> 2015-10-19 Martin Sebor
>
> * init/new45.C: New test to verify that operator new is invoked
> with or without overhead for a cookie.
This new test fails for ARM targets, snipping some of the diff...
> +inline __attribute__ ((always_inline))
> +void* operator new[] (size_t n)
> +{
> +// Verify that array new is invoked with an argument large enough
> +// for the array and a size_t cookie to store the number of elements.
> +// (This holds for classes with user-defined types but not POD types).
> +if (n != N * sizeof (UDClass) + sizeof n) abort ();
> +return malloc (n);
> +}
Cookies on ARM are 8-bytes [1], but sizeof ((size_t) n) is only 4-bytes,
so this check will fail (We'll ask for 500 bytes, the test here will only
be looking for 496).
Would it undermine the test for other architectures if I were to swap out
the != for a >= ? I think that is in line with the "argument large enough
for the array" that this test is looking for, but would not catch bugs where
we were allocating more memory than neccessary.
Otherwise I can spin a patch which skips the test for ARM targets.
Thanks,
James
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
> Cookies on ARM are 8-bytes [1], but sizeof ((size_t) n) is only 4-bytes, > so this check will fail (We'll ask for 500 bytes, the test here will only > be looking for 496). > > Would it undermine the test for other architectures if I were to swap out > the != for a >= ? I think that is in line with the "argument large enough > for the array" that this test is looking for, but would not catch bugs where > we were allocating more memory than neccessary. > > Otherwise I can spin a patch which skips the test for ARM targets. > I didn't want to skip this for ARM, instead something that takes into account the cookie size - (very gratuitous hack was to just add 4 in a #ifdef __arm__ block). Something like attached, brown paper bag warning ;) * g++.dg/init/new45.C: Adjust for cookie size on arm. regards Ramana > Thanks, > James > diff --git a/gcc/testsuite/g++.dg/init/new45.C b/gcc/testsuite/g++.dg/init/new45.C index 92dac18..31473a3 100644 --- a/gcc/testsuite/g++.dg/init/new45.C +++ b/gcc/testsuite/g++.dg/init/new45.C @@ -29,8 +29,16 @@ void* operator new[] (size_t n) // Verify that array new is invoked with an argument large enough // for the array and a size_t cookie to store the number of elements. // (This holds for classes with user-defined types but not POD types). -if (n != N * sizeof (UDClass) + sizeof n) abort (); -return malloc (n); + size_t val = N * sizeof (UDClass) + sizeof n; + +// On ARM EABI the cookie is always 8 bytes as per Section 3.2.2 +// of http://infocenter.arm.com/help/topic/com.arm.doc.ihi0041d/IHI0041D_cppabi.pdf +#if defined( __arm__) && defined(__ARM_EABI__) + val = val + 4; +#endif + + if (n != val) abort (); + return malloc (n); } inline __attribute__ ((always_inline)) @@ -60,8 +68,16 @@ void* operator new[] (size_t n, UDClass *p) // Verify that placement array new overload for a class type with // a user-defined ctor and dtor is invoked with an argument large // enough for the array and a cookie. -if (n != N * sizeof (UDClass) + sizeof n) abort (); -return p; + size_t val = N * sizeof (UDClass) + sizeof n; + +// On ARM EABI the cookie is always 8 bytes as per Section 3.2.2 +// of http://infocenter.arm.com/help/topic/com.arm.doc.ihi0041d/IHI0041D_cppabi.pdf +#if defined( __arm__) && defined(__ARM_EABI__) + val = val + 4; +#endif + + if (n != val) abort (); + return p; } // UDClassllocate a sufficiently large buffer to construct arrays into.
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
On 11/26/2015 04:33 AM, Ramana Radhakrishnan wrote: Cookies on ARM are 8-bytes [1], but sizeof ((size_t) n) is only 4-bytes, so this check will fail (We'll ask for 500 bytes, the test here will only be looking for 496). Would it undermine the test for other architectures if I were to swap out the != for a >= ? I think that is in line with the "argument large enough for the array" that this test is looking for, but would not catch bugs where we were allocating more memory than neccessary. Otherwise I can spin a patch which skips the test for ARM targets. I didn't want to skip this for ARM, instead something that takes into account the cookie size - (very gratuitous hack was to just add 4 in a #ifdef __arm__ block). Something like attached, brown paper bag warning ;) Thanks. I'll commit it today after some testing. I should probably also check to see if there are other such targets and try to find a way to generalize the test. (There should be a way to expose the cookie size to programs, otherwise they have no way to avoid buffer overflow in array forms of placement new). Martin
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
OK. Jason
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
On 11/02/2015 09:55 PM, Jason Merrill wrote:
On 10/26/2015 10:06 PM, Martin Sebor wrote:
+ if (TREE_CONSTANT (maybe_constant_value (outer_nelts)))
+{
+ if (tree_int_cst_lt (max_outer_nelts_tree, outer_nelts))
maybe_constant_value may return a constant, but that doesn't mean that
outer_nelts was already constant; if it wasn't, the call to
tree_int_cst_lt will fail.
Thanks for the hint. I wasn't able to trigger the failure. I suspect
outer_nelts must have already been folded at this point because the
maybe_constant_value call isn't necessary. I removed it.
Since we're moving toward delayed folding, I'd prefer to use the result
of maybe_constant_value only for this diagnostic, and then continue to
pass the unfolded value along.
Sure. Done in the attached patch.
Martin
gcc/cp/ChangeLog
2015-10-19 Martin Sebor
PR c++/67913
PR c++/67927
* call.c (build_operator_new_call): Do not assume size_check
is non-null, analogously to the top half of the function.
* init.c (build_new_1): Detect and diagnose array sizes in
excess of the maximum of roughly SIZE_MAX / 2.
Insert a runtime check only for arrays with a non-constant size.
(build_new): Detect and diagnose negative array sizes.
gcc/testsuite/ChangeLog
2015-10-19 Martin Sebor
* init/new45.C: New test to verify that operator new is invoked
with or without overhead for a cookie.
PR c++/67927
* init/new44.C: New test for placement new expressions for arrays
with excessive number of elements.
PR c++/67913
* init/new43.C: New test for placement new expressions for arrays
with negative number of elements.
* other/new-size-type.C: Expect array new expression with
an excessive number of elements to be rejected.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 367d42b..3f76198 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4228,10 +4228,12 @@ build_operator_new_call (tree fnname, vec **args,
{
/* Update the total size. */
*size = size_binop (PLUS_EXPR, original_size, *cookie_size);
+ if (size_check)
+ {
/* 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. */
(**args)[0] = *size;
}
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 1ed8f6c..3b88098 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2272,7 +2272,11 @@ throw_bad_array_new_length (void)
/* Generate code for a new-expression, including calling the "operator
new" function, initializing the object, and, if an exception occurs
during construction, cleaning up. The arguments are as for
- build_raw_new_expr. This may change PLACEMENT and INIT. */
+ build_raw_new_expr. This may change PLACEMENT and INIT.
+ TYPE is the type of the object being constructed, possibly an array
+ of NELTS elements when NELTS is non-null (in "new T[NELTS]", T may
+ be an array of the form U[inner], with the whole expression being
+ "new U[NELTS][inner]"). */
static tree
build_new_1 (vec **placement, tree type, tree nelts,
@@ -2292,13 +2296,16 @@ build_new_1 (vec **placement, tree type, tree nelts,
type.) */
tree pointer_type;
tree non_const_pointer_type;
+ /* The most significant array bound in int[OUTER_NELTS][inner]. */
tree outer_nelts = NULL_TREE;
- /* For arrays, a bounds checks on the NELTS parameter. */
+ /* For arrays with a non-constant number of elements, a bounds checks
+ on the NELTS parameter to avoid integer overflow at runtime. */
tree outer_nelts_check = NULL_TREE;
bool outer_nelts_from_type = false;
+ /* Number of the "inner" elements in "new T[OUTER_NELTS][inner]". */
offset_int inner_nelts_count = 1;
tree alloc_call, alloc_expr;
- /* Size of the inner array elements. */
+ /* Size of the inner array elements (those with constant dimensions). */
offset_int inner_size;
/* The address returned by the call to "operator new". This node is
a VAR_DECL and is therefore reusable. */
@@ -2492,21 +2499,41 @@ build_new_1 (vec **placement, tree type, tree nelts,
}
max_outer_nelts = wi::udiv_trunc (max_size, inner_size);
- /* Only keep the top-most seven bits, to simplify encoding the
- constant in the instruction stream. */
+ max_outer_nelts_tree = wide_int_to_tree (sizetype, max_outer_nelts);
+
+ size = size_binop (MULT_EXPR, size, convert (sizetype, nelts));
+
+ if (TREE_CONSTANT (outer_nelts))
+ {
+ if (tree_int_cst_lt (max_outer_nelts_tree, outer_nelts))
+ {
+ /* When the array size is constant, check it at compile time
+ to make sure it doesn't exceed the implementation-defined
+ maximum, as required by C++ 14 (in C++ 11 this requirement
+ isn't explicitly stated but it's enforced anyway -- see
+
Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
On 10/26/2015 10:06 PM, Martin Sebor wrote:
+ if (TREE_CONSTANT (maybe_constant_value (outer_nelts)))
+ {
+ if (tree_int_cst_lt (max_outer_nelts_tree, outer_nelts))
maybe_constant_value may return a constant, but that doesn't mean that
outer_nelts was already constant; if it wasn't, the call to
tree_int_cst_lt will fail.
+ tree nelts_save = nelts;
+ nelts = maybe_constant_value (nelts);
+
+ if (!TREE_CONSTANT (nelts))
+ nelts = nelts_save;
+
+ /* The expression in a noptr-new-declarator is erroneous if it's of
+non-class type and its value before converting to std::size_t is
+less than zero. ... If the expression is a constant expression,
+the program is ill-fomed. */
+ if (TREE_CONSTANT (nelts) && tree_int_cst_sgn (nelts) == -1)
+ {
+ if (complain & tf_error)
+ error ("size of array is negative");
+ return error_mark_node;
+ }
Since we're moving toward delayed folding, I'd prefer to use the result
of maybe_constant_value only for this diagnostic, and then continue to
pass the unfolded value along.
Jason
[PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements
[CC Jason] The patch is at the link below: https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01803.html Thanks On 10/19/2015 12:50 PM, Martin Sebor wrote: This is a patch for two C++ bugs: 67913 - new expression with negative size not diagnosed 67927 - array new expression with excessive number of elements not diagnosed The C++ front end rejects a subset of array declarators with negative bounds or with bounds in excess of some implementation defined maximum (roughly SIZE_MAX / 2), but it does so inconsistently. For example, it silently accepts expressions such as new int [-1][2]; but invokes operator new to allocate SIZE_MAX bytes. When operator new succeeds (as is the case on Linux with memory overcommitment enabled), the new expression returns a valid pointer to some unknown amount of storage less than SIZE_MAX. Accessing the memory at some non-zero offset then causes a SIGSEGV. Similarly, GCC accepts the following expression with the same result: new int [SIZE_MAX][2]; C++ 14 makes it clear that such expressions are ill-formed and must be rejected. This patch adds checks that consistently reject all new expressions with both negative array bounds and bounds in excess of the maximum. While I raised these bugs as separate issues I decided to group the two sets of changes together since they both touch the same function in similar ways, and hopefully doing so will make them also easier to review. I've tested the patch by bootstrapping C/C++ and running the test suites (including libstdc++) on x86_64 with no regressions. During the development of the changes I found a few basic mistakes in my code only after running libstdc++ tests. To make it possible to uncover them sooner in the future, I added another test that isn't directly related to the problem: new45.C. I also found a minor problem in the GCC regression test suite where the g++.dg/other/new-size-type.C test for PR 36741 tried to check that the 'new char[~static_cast(0)]' expression was accepted without a warning. The complaint in the PR was about the wording of the warning, not about the validity of the expression (the submitter agreed that a correctly worded diagnostic would be appropriate). I chaged the test to expect a meaningful error message. Once this patch is approved and committed a follow-up patch should document the implementation-defined maximum to the manual. Martin
