This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]