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

[Martin Sebor]

On 11/06/2015 05:50 AM, Andreas Schwab wrote:

I see this failure on m68k:

FAIL: g++.dg/warn/Wplacement-new-size.C  -std=gnu++11 (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:189:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:191:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:194:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:198:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]

That appears to be a 32-bit problem, the test also fails here on x86-64
with -m32 
or here on powerpc



This should be fixed now via r229959 (tested on x86_64 with -m32).

The problem was caused by assuming that the POINTER_PLUS_EXPR offset
which is stored as sizetype, an unsigned 32-bit type in ILP32, can
be "extracted" as an unsigned HOST_WIDE_INT (a 64-bit type when
the host compiler is LP64), and converted to signed to obtain the
original negative offset.

Martin

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

[Martin Sebor]

On 11/06/2015 05:55 AM, Rainer Orth wrote:

Martin Sebor  writes:


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


Okay. Attached is an updated patch with that change.


Unfortunately, this breaks i386-pc-solaris2.10 bootstrap:

/vol/gcc/src/hg/trunk/local/gcc/cp/init.c: In function 'void 
warn_placement_new_too_small(tree, tree, tree, tree)':
/vol/gcc/src/hg/trunk/local/gcc/cp/init.c:2454:17: error: format '%lu' expects 
argument of type 'long unsigned int', but argument 5 has type 'long long 
unsigned int' [-Werror=format=]
   bytes_avail);
  ^

Printing an unsigned HOST_WIDE_INT with %lu in one case, but %wu in the
other seems like a simple typo, so the following fixes bootstrap for me:


Yes, that was a typo. Sorry about that and thanks to ktkachov for
committing the fix!

Martin

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

[Andreas Schwab]

I see this failure on m68k:

FAIL: g++.dg/warn/Wplacement-new-size.C  -std=gnu++11 (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:189:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:191:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:194:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]
/daten/aranym/gcc/gcc-20151106/gcc/testsuite/g++.dg/warn/Wplacement-new-size.C:198:19:
 warning: placement new constructing an object of type 'int' and size '4' in a 
region of type 'char [4]' and size '0' [-Wplacement-new]

That appears to be a 32-bit problem, the test also fails here on x86-64
with -m32 
or here on powerpc


Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Rainer Orth]

Martin Sebor  writes:

>> If we use gcc_checking_assert it won't fire in release builds; let's go
>> with that.
>
> Okay. Attached is an updated patch with that change.

Unfortunately, this breaks i386-pc-solaris2.10 bootstrap:

/vol/gcc/src/hg/trunk/local/gcc/cp/init.c: In function 'void 
warn_placement_new_too_small(tree, tree, tree, tree)':
/vol/gcc/src/hg/trunk/local/gcc/cp/init.c:2454:17: error: format '%lu' expects 
argument of type 'long unsigned int', but argument 5 has type 'long long 
unsigned int' [-Werror=format=]
  bytes_avail);
 ^

Printing an unsigned HOST_WIDE_INT with %lu in one case, but %wu in the
other seems like a simple typo, so the following fixes bootstrap for me:

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2447,7 +2447,7 @@ warn_placement_new_too_small (tree type,
 			  "%<%T [%wu]%> and size %qwu in a region of type %qT "
 			  "and size %qwi"
 			  : "placement new constructing an object of type "
-			  "%<%T [%lu]%> and size %qwu in a region of type %qT "
+			  "%<%T [%wu]%> and size %qwu in a region of type %qT "
 			  "and size at most %qwu",
 			  type, tree_to_uhwi (nelts), bytes_need,
 			  TREE_TYPE (oper),

Rainer


-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

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

[Martin Sebor]

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


Okay. Attached is an updated patch with that change.

Martin

gcc ChangeLog
2015-11-05  Martin Sebor  

	PR c++/67942
* invoke.texi (-Wplacement-new): Document new option.
	* gcc/testsuite/g++.dg/warn/Wplacement-new-size.C: New test.

gcc/c-family ChangeLog
2015-11-05  Martin Sebor  

	PR c++/67942
* c.opt (-Wplacement-new): New option.

gcc/cp ChangeLog
2015-11-05  Martin Sebor  

	PR c++/67942
	* cp/init.c (warn_placement_new_too_small): New function.
	(build_new_1): Call it.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 47ba070..5e9d7a3 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -760,6 +760,10 @@ Wprotocol
 ObjC ObjC++ Var(warn_protocol) Init(1) Warning
 Warn if inherited methods are unimplemented

+Wplacement-new
+C++ Var(warn_placement_new) Init(1) Warning
+Warn for placement new expressions with undefined behavior
+
 Wredundant-decls
 C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
 Warn about multiple declarations of the same object
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 1ed8f6c..3a9c59d 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2269,6 +2269,201 @@ throw_bad_array_new_length (void)
   return build_cxx_call (fn, 0, NULL, tf_warning_or_error);
 }

+/* Attempt to verify that the argument, OPER, of a placement new expression
+   refers to an object sufficiently large for an object of TYPE or an array
+   of NELTS of such objects when NELTS is non-null, and issue a warning when
+   it does not.  SIZE specifies the size needed to construct the object or
+   array and captures the result of NELTS * sizeof (TYPE). (SIZE could be
+   greater when the array under construction requires a cookie to store
+   NELTS.  GCC's placement new expression stores the cookie when invoking
+   a user-defined placement new operator function but not the default one.
+   Placement new expressions with user-defined placement new operator are
+   not diagnosed since we don't know how they use the buffer (this could
+   be a future extension).  */
+static void
+warn_placement_new_too_small (tree type, tree nelts, tree size, tree oper)
+{
+  location_t loc = EXPR_LOC_OR_LOC (oper, input_location);
+
+  /* The number of bytes to add to or subtract from the size of the provided
+ buffer based on an offset into an array or an array element reference.
+ Although intermediate results may be negative (as in a[3] - 2) the final
+ result cannot be.  */
+  HOST_WIDE_INT adjust = 0;
+  /* True when the size of the entire destination object should be used
+ to compute the possibly optimistic estimate of the available space.  */
+  bool use_obj_size = false;
+  /* True when the reference to the destination buffer is an ADDR_EXPR.  */
+  bool addr_expr = false;
+
+  STRIP_NOPS (oper);
+
+  /* Using a function argument or a (non-array) variable as an argument
+ to placement new is not checked since it's unknown what it might
+ point to.  */
+  if (TREE_CODE (oper) == PARM_DECL
+  || TREE_CODE (oper) == VAR_DECL
+  || TREE_CODE (oper) == COMPONENT_REF)
+return;
+
+  /* Evaluate any constant expressions.  */
+  size = fold_non_dependent_expr (size);
+
+  /* Handle the common case of array + offset expression when the offset
+ is a constant.  */
+  if (TREE_CODE (oper) == POINTER_PLUS_EXPR)
+{
+  /* If the offset is comple-time constant, use it to compute a more
+	 accurate estimate of the size of the buffer.  Otherwise, use
+	 the size of the entire array as an optimistic estimate (this
+	 may lead to false negatives).  */
+  const_tree adj = TREE_OPERAND (oper, 1);
+  if (CONSTANT_CLASS_P (adj))
+	adjust += tree_to_uhwi (adj);
+  else
+	use_obj_size = true;
+
+  oper = TREE_OPERAND (oper, 0);
+
+  STRIP_NOPS (oper);
+}
+
+  if (TREE_CODE (oper) == TARGET_EXPR)
+oper = TREE_OPERAND (oper, 1);
+  else if (TREE_CODE (oper) == ADDR_EXPR)
+{
+  addr_expr = true;
+  oper = TREE_OPERAND (oper, 0);
+}
+
+  STRIP_NOPS (oper);
+
+  if (TREE_CODE (oper) == ARRAY_REF)
+{
+  /* Similar to the offset computed above, see if the array index
+	 is a compile-time constant.  If so, and unless the offset was
+	 not a compile-time constant, use the index to determine the
+	 size of the buffer.  Otherwise, use the entire array as
+	 an optimistic estimate of the size.  */
+  const_tree adj = TREE_OPERAND (oper, 1);
+  if (!use_obj_size && CONSTANT_CLASS_P (adj))
+	adjust += tree_to_shwi (adj);
+  else
+	{
+	  use_obj_size = true;
+	  adjust = 0;
+	}
+
+  oper = TREE_OPERAND (oper, 0);
+}
+
+  /* Descend into a struct or union to find the member whose address
+ is being used as the agument.  */
+  while (TREE_CODE (oper) == COMPONENT_REF)
+oper = TREE_OPERAND (oper, 1);
+
+  if ((addr_expr || !POINTER_TYPE_P (TREE_TYPE (oper)))
+   

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

[Jason Merrill]

OK, thanks.

Jason

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

[Martin Sebor]

On 11/05/2015 11:13 AM, Jason Merrill wrote:

OK, thanks.


I'm afraid the last patch that I just committed breaks libstdc++
bootstrap with the following error:

/home/msebor/scm/fsf/gcc-svn/libstdc++-v3/src/c++98/ios_init.cc: In 
static member function ‘static bool std::ios_base::sync_with_stdio(bool)’:
/home/msebor/scm/fsf/gcc-svn/libstdc++-v3/src/c++98/ios_init.cc:181:59: 
internal compiler error: in tree_to_shwi, at tree.c:7297

  new (_cout) stdio_filebuf(stdout, ios_base::out);

I thought it had something to do with the recent changes made to
the patch but it looks like it's a problem that's been there all
along: the patch assumes DECL_SIZE_UNIT (oper) is non-null but
it turns out that's not a safe assumption. I don't know why my
testing didn't expose it before, despite running make bootstrap
and make check.

The following patch (only the first bit is important) fixes the
ICE. I will commit it as soon as I'm done testing it.

Sorry about the breakage.

Martin

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 7600363..ca7de56 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2384,12 +2384,13 @@ warn_placement_new_too_small (tree type, tree 
nelts, tree size, tree oper)

   /* Treat members of unions and members of structs uniformly, even
 though the size of a member of a union may be viewed as extending
 to the end of the union itself (it is by 
__builtin_object_size).  */

-  if (TREE_CODE (oper) == VAR_DECL || use_obj_size)
+  if ((TREE_CODE (oper) == VAR_DECL || use_obj_size)
+ && (DECL_SIZE_UNIT (oper)))
{
  /* Use the size of the entire array object when the expression
 refers to a variable or its size depends on an expression
 that's not a compile-time constant.  */
- bytes_avail = tree_to_shwi (DECL_SIZE_UNIT (oper));
+ bytes_avail = tree_to_uhwi (DECL_SIZE_UNIT (oper));
  exact_size = !use_obj_size;
}
   else

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

[Martin Sebor]

On 11/05/2015 04:41 PM, Martin Sebor wrote:

On 11/05/2015 11:13 AM, Jason Merrill wrote:

OK, thanks.


I'm afraid the last patch that I just committed breaks libstdc++
bootstrap with the following error:


I committed a slightly more robust fix that avoids assuming that
either the size of the object or its type is known.  Those were
both bugs in the patch.

What seems unexpected to me that given the following declarations,
DECL_SIZE_UNIT(s) is null but TYPE_SIZE_UNIT (TREE_TYPE (s)) is
not.  The code is derived from the libstdc++ translation unit that
caused the ICE.  Is it because GCC conservatively assumes that
S could be explicitly specialized in another translation
unit?  If so, then I would expect TYPE_SIZE_UNIT (TREE_TYPE (s))
to also be null.  What am I missing?

  template  struct S { int i; };

  extern struct S s;

Martin

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

[Jason Merrill]

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 () 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 ( [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

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

[Martin Sebor]

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 () 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 ( [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).

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.

Thanks
Martin

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

[Martin Sebor]

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 () 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 ( [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.

Tested on x86_64 by botstrapping C and C++ and running make check.

Martin
gcc ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
* invoke.texi (-Wplacement-new): Document new option.
	* gcc/testsuite/g++.dg/warn/Wplacement-new-size.C: New test.

gcc/c-family ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
* c.opt (-Wplacement-new): New option.

gcc/cp ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
	* cp/init.c (warn_placement_new_too_small): New function.
	(build_new_1): Call it.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 47ba070..5e9d7a3 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -760,6 +760,10 @@ Wprotocol
 ObjC ObjC++ Var(warn_protocol) Init(1) Warning
 Warn if inherited methods are unimplemented

+Wplacement-new
+C++ Var(warn_placement_new) Init(1) Warning
+Warn for placement new expressions with undefined behavior
+
 Wredundant-decls
 C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
 Warn about multiple declarations of the same object
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 1ed8f6c..e2285ec 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2269,6 +2269,199 @@ throw_bad_array_new_length (void)
   return build_cxx_call (fn, 0, NULL, tf_warning_or_error);
 }

+/* Attempt to verify that the argument, OPER, of a placement new expression
+   refers to an object sufficiently large for an object of TYPE or an array
+   of NELTS of such objects when NELTS is non-null, and issue a warning when
+   it does not.  SIZE specifies the size needed to construct the object or
+   array and captures the result of NELTS * sizeof (TYPE). (SIZE could be
+   greater when the array under construction requires a cookie to store
+   NELTS.  GCC's placement new expression stores the cookie when invoking
+   a user-defined placement new operator function but not the default one.
+   Placement new expressions with user-defined placement new operator are
+   not diagnosed since we don't know how they use the buffer (this could
+   be a future extension).  */
+static void
+warn_placement_new_too_small (tree type, tree nelts, tree size, tree oper)
+{
+  location_t loc = EXPR_LOC_OR_LOC (oper, input_location);
+
+  /* The number of bytes to subtract from the size of the provided buffer
+ based on an offset into an array or an array element reference.
+ Although intermediate results may be negative (as in a[3] - 2),
+ the ultimate result cannot be and so the computation is done in
+ unsigned HOST_WIDE_INT.  */
+  unsigned HOST_WIDE_INT adjust = 0;
+  /* True when the size of the entire destination object should be used
+ to compute the possibly optimistic estimate of the available space.  */
+  bool use_obj_size = false;
+  /* True when the reference to the destination buffer is an ADDR_EXPR.  */
+  bool addr_expr = false;
+
+  STRIP_NOPS (oper);
+
+  /* Using a function argument or a (non-array) variable as an argument
+ to placement new is not checked since it's unknown what it might
+ point to.  */
+  if (TREE_CODE (oper) == PARM_DECL
+  || TREE_CODE (oper) == VAR_DECL
+  || TREE_CODE (oper) == COMPONENT_REF)
+return;
+
+  /* Evaluate any constant expressions.  */
+  size = fold_non_dependent_expr (size);
+
+  /* Handle the common case of array + offset expression when the offset
+ is a constant.  */
+  if (TREE_CODE (oper) == POINTER_PLUS_EXPR)
+{
+  /* If the offset is comple-time constant, use it to compute a more
+	 accurate estimate of the size of the buffer.  Otherwise, use
+	 the size of the entire array as an optimistic estimate (this
+	 may lead to false negatives).  */
+  const_tree adj = TREE_OPERAND (oper, 1);
+  if (CONSTANT_CLASS_P (adj))
+	adjust += tree_to_uhwi (adj);
+  else
+	use_obj_size = true;
+
+  oper = TREE_OPERAND (oper, 0);
+
+  STRIP_NOPS (oper);
+}
+
+  if (TREE_CODE (oper) == TARGET_EXPR)
+oper = TREE_OPERAND (oper, 1);
+  else if (TREE_CODE (oper) == ADDR_EXPR)
+{

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

[Jason Merrill]

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 () 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 ( [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.


Jason

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

[Jason Merrill]

On 11/04/2015 12:11 PM, Martin Sebor wrote:

On 11/02/2015 07:40 PM, Jason Merrill wrote:

On 10/26/2015 09:48 PM, Martin Sebor wrote:

+  while (TREE_CODE (oper) == NOP_EXPR)
+oper = TREE_OPERAND (oper, 0);


This is STRIP_NOPS.


+ to placement new is not checked since it's unknownwhat it might


Missing space.


+  else if (TREE_CODE (oper) == ADDR_EXPR) {


The brace should go on its own line.


+  /* A possibly optimistic estimate Number of bytes available


Maybe "of the number"?


Thanks for the review. I've fixed the issues above in the latest
patch (attached).




+  /* When the referenced object is a member of a union, use the
size
+ of the entire union as the size of the buffer.  */


Why?  If we're accessing one union member, we should limit the allowed
space to the size of that member.


I followed the more permissive approach taken by _FORTIFY_SOURCE
where the size of the whole object is used (on the assumption that
we will eventually want to adopt the same mechanism here). For
instance, given:

   union U { char c; int i; } u;

GCC doesn't diagnose:

   memset (, 0, sizeof u);

so I didn't expect we'd want the following diagnosed either:

   new () U ();

But if you think it's preferable to use the size of the member
for this iteration of the placement new warning I can change it.
Can you confirm?


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 () if that's what 
they mean.



+  if (bytes_avail <= abs (adjust))
+bytes_avail = 0;
+  else if (0 <= adjust)
+bytes_avail -= adjust;
+  else
+bytes_avail += adjust;


If adjust is negative, I would think that we would have returned already
because we were dealing with an offset from a pointer of unknown value.


Adjust is negative when the offset to a buffer of known size is
negative. For example:

 char buf [sizeof (int)];
 new ( [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.



It also seems that you're being careful to avoid bytes_avail going
negative, so I wonder why you have it signed and bytes_need unsigned.


+  warning_at (EXPR_LOC_OR_LOC (orig_oper, input_location),


Let's remember this location early on so you don't need orig_oper.


Done.

Martin

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

[Martin Sebor]

On 11/02/2015 07:40 PM, Jason Merrill wrote:

On 10/26/2015 09:48 PM, Martin Sebor wrote:

+  while (TREE_CODE (oper) == NOP_EXPR)
+oper = TREE_OPERAND (oper, 0);


This is STRIP_NOPS.


+ to placement new is not checked since it's unknownwhat it might


Missing space.


+  else if (TREE_CODE (oper) == ADDR_EXPR) {


The brace should go on its own line.


+  /* A possibly optimistic estimate Number of bytes available


Maybe "of the number"?


Thanks for the review. I've fixed the issues above in the latest
patch (attached).




+  /* When the referenced object is a member of a union, use the size
+ of the entire union as the size of the buffer.  */


Why?  If we're accessing one union member, we should limit the allowed
space to the size of that member.


I followed the more permissive approach taken by _FORTIFY_SOURCE
where the size of the whole object is used (on the assumption that
we will eventually want to adopt the same mechanism here). For
instance, given:

  union U { char c; int i; } u;

GCC doesn't diagnose:

  memset (, 0, sizeof u);

so I didn't expect we'd want the following diagnosed either:

  new () U ();

But if you think it's preferable to use the size of the member
for this iteration of the placement new warning I can change it.
Can you confirm?




+  if (bytes_avail <= abs (adjust))
+bytes_avail = 0;
+  else if (0 <= adjust)
+bytes_avail -= adjust;
+  else
+bytes_avail += adjust;


If adjust is negative, I would think that we would have returned already
because we were dealing with an offset from a pointer of unknown value.


Adjust is negative when the offset to a buffer of known size is
negative. For example:

char buf [sizeof (int)];
new ( [1] - 1) int;



It also seems that you're being careful to avoid bytes_avail going
negative, so I wonder why you have it signed and bytes_need unsigned.


+  warning_at (EXPR_LOC_OR_LOC (orig_oper, input_location),


Let's remember this location early on so you don't need orig_oper.


Done.

Martin
gcc ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
* invoke.texi (-Wplacement-new): Document new option.
	* gcc/testsuite/g++.dg/warn/Wplacement-new-size.C: New test.

gcc/c-family ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
* c.opt (-Wplacement-new): New option.

gcc/cp ChangeLog
2015-11-04  Martin Sebor  

	PR c++/67942
	* cp/init.c (warn_placement_new_too_small): New function.
	(build_new_1): Call it.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 47ba070..5e9d7a3 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -760,6 +760,10 @@ Wprotocol
 ObjC ObjC++ Var(warn_protocol) Init(1) Warning
 Warn if inherited methods are unimplemented

+Wplacement-new
+C++ Var(warn_placement_new) Init(1) Warning
+Warn for placement new expressions with undefined behavior
+
 Wredundant-decls
 C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
 Warn about multiple declarations of the same object
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 1ed8f6c..ebf283d 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2269,6 +2269,200 @@ throw_bad_array_new_length (void)
   return build_cxx_call (fn, 0, NULL, tf_warning_or_error);
 }

+/* Attempt to verify that the argument, OPER, of a placement new expression
+   refers to an object sufficiently large for an object of TYPE or an array
+   of NELTS of such objects when NELTS is non-null, and issue a warning when
+   it does not.  SIZE specifies the size needed to construct the object or
+   array and captures the result of NELTS * sizeof (TYPE). (SIZE could be
+   greater when the array under construction requires a cookie to store
+   NELTS.  GCC's placement new expression stores the cookie when invoking
+   a user-defined placement new operator function but not the default one.
+   Placement new expressions with user-defined placement new operator are
+   not diagnosed since we don't know how they use the buffer (this could
+   be a future extension).  */
+static void
+warn_placement_new_too_small (tree type, tree nelts, tree size, tree oper)
+{
+  location_t loc = EXPR_LOC_OR_LOC (oper, input_location);
+
+  /* The number of bytes to add or subtract from the size of the provided
+ buffer based on an offset into an array or an array element reference.  */
+  HOST_WIDE_INT adjust = 0;
+  /* True when the size of the entire destination object should be used
+ to compute the possibly optimistic estimate of the available space.  */
+  bool use_obj_size = false;
+  /* True when the reference to the destination buffer is an ADDR_EXPR.  */
+  bool addr_expr = false;
+
+  STRIP_NOPS (oper);
+
+  /* Using a function argument or a (non-array) variable as an argument
+ to placement new is not checked since it's unknown what it might
+ point to.  */
+  if (TREE_CODE (oper) == PARM_DECL
+  || TREE_CODE (oper) == VAR_DECL
+  || TREE_CODE (oper) == 

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

[Jason Merrill]

On 10/26/2015 09:48 PM, Martin Sebor wrote:

+  while (TREE_CODE (oper) == NOP_EXPR)
+oper = TREE_OPERAND (oper, 0);


This is STRIP_NOPS.


+ to placement new is not checked since it's unknownwhat it might


Missing space.


+  else if (TREE_CODE (oper) == ADDR_EXPR) {


The brace should go on its own line.


+  /* A possibly optimistic estimate Number of bytes available


Maybe "of the number"?


+  /* When the referenced object is a member of a union, use the size
+of the entire union as the size of the buffer.  */


Why?  If we're accessing one union member, we should limit the allowed 
space to the size of that member.



+  if (bytes_avail <= abs (adjust))
+   bytes_avail = 0;
+  else if (0 <= adjust)
+   bytes_avail -= adjust;
+  else
+   bytes_avail += adjust;


If adjust is negative, I would think that we would have returned already 
because we were dealing with an offset from a pointer of unknown value.


It also seems that you're being careful to avoid bytes_avail going 
negative, so I wonder why you have it signed and bytes_need unsigned.



+ warning_at (EXPR_LOC_OR_LOC (orig_oper, input_location),


Let's remember this location early on so you don't need orig_oper.

Jason