Re: [PING 2] [PATCH] c++/67942 - diagnose placement new buffer overflow

[Jason Merrill <jason@redhat.com>]

On 11/05/2015 10:12 AM, Martin Sebor wrote:
> On 11/04/2015 09:27 PM, Jason Merrill wrote:
>> On 11/04/2015 07:15 PM, Martin Sebor wrote:
>>>> There was a lot of discussion of C++ aliasing rules at the recent
>>>> meeting; we really seem to be moving in the direction of being stricter
>>>> about which union member is active.  So I think we do want to diagnose
>>>> the new-expression above; the user should write new (&u) if that's what
>>>> they mean.
>>>
>>> Okay. I changed that in the latest patch.
>>>
>>>>> Adjust is negative when the offset to a buffer of known size is
>>>>> negative. For example:
>>>>>
>>>>>      char buf [sizeof (int)];
>>>>>      new (&buf [1] - 1) int;
>>>>
>>>> OK, so because we're looking at the expression from the outside in, we
>>>> first see the subtraction and adjust becomes -1, then we see the
>>>> array_ref and adjust returns to 0.  We still don't have a negative
>>>> adjust by the time we get to the quoted if/else.
>>>
>>> I think I see what you mean. I've changed the type of the variables
>>> and the computation to unsigned. That made it possible to eliminate
>>> the final else and do some other cleanup. Attached is an updated
>>> patch.
>>
>> Hmm, I was suggesting that bytes_avail change to unsigned, but I don't
>> think adjust should change; I believe that 0u - 1u is undefined due to
>> overflow even though (-1u) and (unsigned)-1 are well defined.  Sorry for
>> the muddled messages.  I think let's leave adjust signed and assert that
>> it ends up non-negative.
>
> No problem.
>
> Unsigned ints wrap around and don't overflow so the subtraction
> is well defined (0u - 1u is equal UINT_MAX).

I thought I had remembered that, but couldn't find anything in the 
standard to back it up.  Now I see that it's in 3.9.1 rather than clause 5.

> FWIW, I had the assert there for sanity testing when you first
> mentioned it to convince myself there really was no way for it
> become negative. A bootstrap went fine with it but it still made
> me just a teeny bit uneasy. I would hate for the code to change
> in the future and for the assert to then fire after it's released.

> In any case, I defer to your better judgment. Please let me know
> if you would still like to go with signed + assert.

If we use gcc_checking_assert it won't fire in release builds; let's go 
with that.

Jason