diff mbox series

emit a trap for buffer overflow in placement new

Message ID 29bc2630-b0c7-472f-f8a6-3cc9dc1185c5@gmail.com
State New
Headers show
Series emit a trap for buffer overflow in placement new | expand

Commit Message

Martin Sebor Dec. 4, 2017, 10:28 p.m. UTC 
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?

Martin

PS I added the tests to the G++ test suite rather than to
libstdc++ because the latter doesn't understand the DejaGnu
scan-tree-dump directive (looks like it's missing an import
for the .exp file that defines it).

Comments

Marc Glisse Dec. 4, 2017, 10:44 p.m. UTC | #1 
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...
Martin Sebor Dec. 4, 2017, 11:56 p.m. UTC | #2 
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.

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.

Still, I wonder if tightening up the standard to require that
the pointer point to an object of at least n bytes in size would
run into problems or be met with objections.

Martin
Marc Glisse Dec. 5, 2017, 8:48 a.m. UTC | #3 
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?

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

> Still, I wonder if tightening up the standard to require that
> the pointer point to an object of at least n bytes in size would
> run into problems or be met with objections.
Jonathan Wakely Dec. 5, 2017, 1:53 p.m. UTC | #4 
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?
>
>>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.
Martin Sebor Dec. 6, 2017, 12:06 a.m. UTC | #5 
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
diff mbox series

Patch

libstdc++-v3/ChangeLog:

	* libsupc++/new (placement new)[_FORTIFY_SOURCE]: Emit
	__builtin_trap.

gcc/ChangeLog:

	* g++.dg/init/new49.C: New test.
	* g++.dg/init/new50.C: New test.

diff --git a/gcc/testsuite/g++.dg/init/new49.C b/gcc/testsuite/g++.dg/init/new49.C
new file mode 100644
index 0000000..5713aef
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new49.C
@@ -0,0 +1,57 @@ 
+// { dg-do compile }
+// { dg-options "-O2 -Wno-placement-new -fdump-tree-optimized" }
+
+#define _FORTIFY_SOURCE 1
+#include <new>
+
+void sink (void*);
+
+void test_placement_new_single_auto ()
+{
+  char buf[2];
+  long *p = new (buf) long;
+  sink (p);
+}
+
+void* test_placement_new_signle_allocated ()
+{
+  char *buf = (char*)::operator new (2);
+  long *p = new (buf) long;
+  return p;
+}
+
+void test_placement_new_array_auto_const ()
+{
+  char buf[sizeof (int)];
+  int *p = new (buf) int[2];
+  sink (p);
+}
+
+void* test_placement_new_array_allocated_const ()
+{
+  char *buf = (char*)::operator new (sizeof (int));
+  int *p = new (buf) int[2];
+  return p;
+}
+
+void test_placement_new_array_auto_range (unsigned n)
+{
+  if (n < 2)
+    n = 2;
+
+  char buf[sizeof (int)];
+  int *p = new (buf) int[n];
+  sink (p);
+}
+
+void test_placement_new_array_allocated_range (unsigned n)
+{
+  if (n < 2)
+    n = 2;
+
+  char *buf = (char*)::operator new (sizeof (int));
+  int *p = new (buf) int[n];
+  sink (p);
+}
+
+// { dg-final { scan-tree-dump-times "__builtin_trap" 6 "optimized" } }
diff --git a/gcc/testsuite/g++.dg/init/new50.C b/gcc/testsuite/g++.dg/init/new50.C
new file mode 100644
index 0000000..1490992
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new50.C
@@ -0,0 +1,22 @@ 
+// { dg-do compile }
+// { dg-options "-O2 -fdump-tree-optimized" }
+
+#include <new>
+
+void sink (void*);
+
+void test_placement_new_single ()
+{
+  char buf[2];
+  long *p = new (buf) long;   // { dg-warning "placement new" }
+  sink (p);
+}
+
+void test_placement_new_array ()
+{
+  char buf[2];
+  int *p = new (buf) int[2];  // { dg-warning "placement new" }
+  sink (p);
+}
+
+// { dg-final { scan-tree-dump-not "__builtin_trap" "optimized" } }
diff --git a/libstdc++-v3/libsupc++/new b/libstdc++-v3/libsupc++/new
index 0e408b1..ef7ea6d 100644
--- a/libstdc++-v3/libsupc++/new
+++ b/libstdc++-v3/libsupc++/new
@@ -165,10 +165,25 @@  void operator delete[](void*, std::size_t, std::align_val_t)
 #endif // __cpp_aligned_new
 
 // Default placement versions of operator new.
-inline void* operator new(std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT
-{ return __p; }
-inline void* operator new[](std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT
-{ return __p; }
+inline void* operator new(std::size_t __n, void* __p) _GLIBCXX_USE_NOEXCEPT
+{
+#ifdef _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
+  if (__builtin_object_size (__p, 0) < __n)
+    __builtin_trap ();
+#endif
+
+  return __p;
+}
+
+inline void* operator new[](std::size_t __n, void* __p) _GLIBCXX_USE_NOEXCEPT
+{
+#ifdef _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
+  if (__builtin_object_size (__p, 0) < __n)
+    __builtin_trap ();
+#endif
+
+  return __p;
+}
 
 // Default placement versions of operator delete.
 inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }