[PR19351, C++] Fix heap overflow in operator new[]

[Richard Guenther]

On Sun, May 29, 2011 at 9:02 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Jason Merrill:
>
>> Sorry it's taken so long to review this.
>
> Same here. *sigh*  Thanks for your comments.
>
>> On 02/21/2011 04:05 PM, Florian Weimer wrote:
>>>  build_operator_new_call (tree fnname, VEC(tree,gc) **args,
>>> -                        tree *size, tree *cookie_size,
>>> +                        tree *size, tree size_with_cookie, tree *cookie_size,
>>
>> We don't need both size_with_cookie and cookie_size here.  Let's
>> change the cookie_size parameter to be size_with_cookie instead (and
>> adjust the places in build_new_1 that check for cookie_size being set
>> or not).
>
> Okay.
>
>>> +  if (!flag_new_overflow_check)
>>> +    return result;
>>
>> Let's check for constant results here.  If we have a TREE_CONSTANT
>> result that overflows, we can handle it even if we aren't emitting the
>> checks for non-constant values.
>
> I assume I can report an error in this case?

Only a warning.  The code is only undefined at runtime.  But you
could convert the allocation to __builtin_trap () (hmm, what
about exceptions? Is such an allocation required to throw?)

Richard.

>> And if we have a TREE_CONSTANT result that doesn't overflow, we
>> don't have to bother building up the complicated trees in order to
>> fold them away.
>
> Understood.  This is sort-of needed ...
>
>>> +    nelts = maybe_build_size_mult_saturated
>>> +      (convert (sizetype, nelts),
>>> +       convert (sizetype, array_type_nelts_top (elt_type)),
>>> +       NULL_TREE);
>>
>> It seems unfortunate to do this runtime checking more than once for
>> multi-dimensional new.  Since the element size and all but one of the
>> array dimensions are going to be constant in most cases, let's try to
>> multiply the element size with the constant dimensions before we bring
>> in the potentially non-constant dimension.
>
> ... to implement this anyway.  I plan to accumulate constant and
> variable factors separately, and multiply them together in the end.
> VARIABLE * VARIABLE will still incur an expensive overflow check
> (involving integer division, even), but there's currently no way
> around that.
>
> I already have something in that direction, but it is still rather
> messy.  Hopefully, it will be a temporary measure only, and this
> burden can go away when the middle-end learns about saturating
> arithmetic, or we implement std::bad_array_new_length from C++0X (and
> have removed VLA support).
>
>>>     I still need some guidance where to put the test case.  There don't
>>>     seem to be any direct tests for operator new[] funcionality.
>>
>> Let's put it in g++.dg/init.
>
> Okay.  Curiously, the test suite has rather poor coverage of operator
> new[] right now.  Multi-dimensional new is also extremely rare in the
> C++ code bases I looked at.
>