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

[Richard Guenther <richard dot guenther at gmail dot com>]

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.
>