This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] emit a trap for buffer overflow in placement new
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