[PATCH] emit a trap for buffer overflow in placement new

[Martin Sebor]

On 12/05/2017 06:53 AM, Jonathan Wakely wrote:
> On 05/12/17 09:48 +0100, Marc Glisse wrote:
>> On Mon, 4 Dec 2017, Martin Sebor wrote:
>>
>>> On 12/04/2017 03:44 PM, Marc Glisse wrote:
>>>> On Mon, 4 Dec 2017, Martin Sebor wrote:
>>>>
>>>>> The -Wplacement-new option warns for buffer overflow in placement
>>>>> new expressions with objects of constant sizes, but because it's
>>>>> implemented completely in the C++ front end it misses the more
>>>>> interesting non-constant sizes.
>>>>>
>>>>> The attached patch instruments both forms of operator placement
>>>>> new to emit a trap when __builtin_object_size() determines that
>>>>> the pointer points to an object less than the specified number
>>>>> of bytes.  This is done only when _FORTIFY_SOURCE is defined
>>>>> to a non-zero value.  This makes it possible to prevent buffer
>>>>> overflow in most of the same cases as in built-ins like strcpy,
>>>>> though without warnings when the size is nor a C++ constant
>>>>> integer expression.
>>>>>
>>>>> On x86_64-linux it passes testing with no apparent regressions.
>>>>> Can anyone think of problems with this solution?  If not, given
>>>>> its simplicity, would it be appropriate even at this stage?
>>>>
>>>> AFAIK, one can call this operator new manually on any pointer,
>>>> including
>>>> one-past-the-end pointers and null pointers. It is only with new
>>>> expressions that the limitation comes in (because it runs a constructor
>>>> afterwards). Not that people often do that...
>>>
>>> Hmm, yes, there don't seem to be any requirements on calling
>>> the operator by itself.  That's too bad.  I was going to move
>>> the placement new checking into the middle-end to improve
>>> the detection and avoid some false positives but the operator
>>> disappears too early, before the size of the expression that's
>>> being stuffed into the destination is known.
>>
>> What's wrong with generating the same code you wanted to put in
>> operator new from the front-end, next to the call to operator new,
>> when the front-end handles a new expression?

I think it's a reasonable suggestion, thanks.  The only wrinkle
in it is that the trap is guarded by _FORTIFY_SOURCE, so if we
wanted the same behavior as Glibc provides for the string
functions, the C++ front end would have to check for it.  I'm
not sure how favorably hardwiring a Glibc macro into the compiler
would be looked upon.  I suspect not very, so unless it was okay
to trap unconditionally, some other mechanism would be needed
to control it.  Maybe a new option, such as -ftrap-undefined,
to control this behavior throughout GCC.

When changing the compiler I would also want to issue a warning
and not just trap (the trap in the patch I sent was obviously
suboptimal from that point of view).  So I think in the end,
the C++ front end will need to insert both a (conditional) trap
and a builtin to warn on expansion.  The two might be separate
or a single builtin for simplicity.  But I'm just brainstorming
now.

>>> The only other solution that comes to mind to detect non-constant
>>> overflow is to do something like:
>>>
>>> void* operator new (size_t n, void *p)
>>> {
>>>   if (__builtin_object_size (p, 0) < n)
>>>    __builtin_warn ("buffer overflow in placement new");
>>>   return p;
>>> }
>>>
>>> and emit the warning from __builtin_warn during expansion.  That
>>> should work and be usable in other contexts as well (e.g., to
>>> implement overflow and other error detection in user-defined
>>> functions).  It would be kind of like the middle-end form of
>>> Clang attribute diagnose_if.
>>
>> I may have misunderstood, but that seems to have the same issue of
>> applying to all calls to operator new instead of just new expressions.
>
> Yes, but if it's only a warning then at least it doesn't cause
> compilation to fail in the (probably very rare) cases where people are
> doing something strange and calling it directly with a too-small
> pointer.

Right.  I have never seen direct calls to placement new and
I can't think of anything they could be good for, but there
could very well be some.  If someone knows of a valid use
case I'd be interested in hearing about it.

Martin