[RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Jakub Jelinek]

Hi!

As mentioned in PR49905, -D_FORTIFY_SOURCE{,=2} handles e.g.
malloc (4) or malloc (16) well, knowing that the resulting pointer
has object size 4 resp. 16, but for new int or new int[4], it currently
doesn't assume anything (i.e. __builtin_object_size (new int, 0) returns
-1).  While I see the C++ standard unfortunately allows redefining
of the new and vector new operators, I wonder if for -D_FORTIFY_SOURCE
we could assume similar properties as for malloc for the object size
checking, i.e. that if these two operators are called with a constant
parameter, the object size allocated is the given size.  I hope there
aren't C++ programs that override the default operator new, allocate fewer
or more bytes and expect that those can be accessed through the pointer
returned by new.  At least -D_FORTIFY_SOURCE=2 is declared to be stricter
than the standard (but -D_FORTIFY_SOURCE=1 is not).  Of course this wouldn't
affect programs not compiled with -D_FORTIFY_SOURCE{,=2}, wouldn't affect
placement new nor any class operator new/new[] (unless it calls the default
operator new/new[]).

Comments?

2011-08-03  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/49905
	* decl.c (cxx_init_decl_processing): Add alloc_size (1) attribute
	for operator new and operator new [].

	* g++.dg/ext/builtin-object-size3.C: New test.

--- gcc/cp/decl.c.jj	2011-07-22 22:14:59.000000000 +0200
+++ gcc/cp/decl.c	2011-08-03 14:00:48.000000000 +0200
@@ -3629,6 +3629,7 @@ cxx_init_decl_processing (void)
   current_lang_name = lang_name_cplusplus;
 
   {
+    tree newattrs;
     tree newtype, deltype;
     tree ptr_ftype_sizetype;
     tree new_eh_spec;
@@ -3656,7 +3657,11 @@ cxx_init_decl_processing (void)
     else
       new_eh_spec = noexcept_false_spec;
 
-    newtype = build_exception_variant (ptr_ftype_sizetype, new_eh_spec);
+    newattrs
+      = build_tree_list (get_identifier ("alloc_size"),
+			 build_tree_list (NULL_TREE, integer_one_node));
+    newtype = cp_build_type_attribute_variant (ptr_ftype_sizetype, newattrs);
+    newtype = build_exception_variant (newtype, new_eh_spec);
     deltype = build_exception_variant (void_ftype_ptr, empty_except_spec);
     push_cp_library_fn (NEW_EXPR, newtype);
     push_cp_library_fn (VEC_NEW_EXPR, newtype);
--- gcc/testsuite/g++.dg/ext/builtin-object-size3.C.jj	2011-08-03 14:06:03.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/builtin-object-size3.C	2011-08-03 14:04:21.000000000 +0200
@@ -0,0 +1,26 @@
+// { dg-do compile }
+// { dg-options "-O2" }
+
+void baz (int *, int *);
+
+#define MEMCPY(d,s,l) __builtin___memcpy_chk (d, s, l, __builtin_object_size (d, 0))
+
+int
+foo ()
+{
+  int *p = new int;
+  int *q = new int[4];
+  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int));
+  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int));
+  baz (p, q);
+}
+
+int
+bar ()
+{
+  int *p = new int;
+  int *q = new int[4];
+  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int) + 1);		// { dg-warning "will always overflow destination buffer" }
+  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int) + 1);	// { dg-warning "will always overflow destination buffer" }
+  baz (p, q);
+}

	Jakub

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Richard Guenther]

On Wed, Aug 3, 2011 at 2:31 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As mentioned in PR49905, -D_FORTIFY_SOURCE{,=2} handles e.g.
> malloc (4) or malloc (16) well, knowing that the resulting pointer
> has object size 4 resp. 16, but for new int or new int[4], it currently
> doesn't assume anything (i.e. __builtin_object_size (new int, 0) returns
> -1).  While I see the C++ standard unfortunately allows redefining
> of the new and vector new operators, I wonder if for -D_FORTIFY_SOURCE
> we could assume similar properties as for malloc for the object size
> checking, i.e. that if these two operators are called with a constant
> parameter, the object size allocated is the given size.  I hope there
> aren't C++ programs that override the default operator new, allocate fewer
> or more bytes and expect that those can be accessed through the pointer
> returned by new.  At least -D_FORTIFY_SOURCE=2 is declared to be stricter
> than the standard (but -D_FORTIFY_SOURCE=1 is not).  Of course this wouldn't
> affect programs not compiled with -D_FORTIFY_SOURCE{,=2}, wouldn't affect
> placement new nor any class operator new/new[] (unless it calls the default
> operator new/new[]).
>
> Comments?

If that's reasonable then adding the malloc attribute should be, too.
Finally.  Please.  Doesn't C++0x maybe "fix" the issue we were
discussing to death?

Richard.

> 2011-08-03  Jakub Jelinek  <jakub@redhat.com>
>
>        PR middle-end/49905
>        * decl.c (cxx_init_decl_processing): Add alloc_size (1) attribute
>        for operator new and operator new [].
>
>        * g++.dg/ext/builtin-object-size3.C: New test.
>
> --- gcc/cp/decl.c.jj    2011-07-22 22:14:59.000000000 +0200
> +++ gcc/cp/decl.c       2011-08-03 14:00:48.000000000 +0200
> @@ -3629,6 +3629,7 @@ cxx_init_decl_processing (void)
>   current_lang_name = lang_name_cplusplus;
>
>   {
> +    tree newattrs;
>     tree newtype, deltype;
>     tree ptr_ftype_sizetype;
>     tree new_eh_spec;
> @@ -3656,7 +3657,11 @@ cxx_init_decl_processing (void)
>     else
>       new_eh_spec = noexcept_false_spec;
>
> -    newtype = build_exception_variant (ptr_ftype_sizetype, new_eh_spec);
> +    newattrs
> +      = build_tree_list (get_identifier ("alloc_size"),
> +                        build_tree_list (NULL_TREE, integer_one_node));
> +    newtype = cp_build_type_attribute_variant (ptr_ftype_sizetype, newattrs);
> +    newtype = build_exception_variant (newtype, new_eh_spec);
>     deltype = build_exception_variant (void_ftype_ptr, empty_except_spec);
>     push_cp_library_fn (NEW_EXPR, newtype);
>     push_cp_library_fn (VEC_NEW_EXPR, newtype);
> --- gcc/testsuite/g++.dg/ext/builtin-object-size3.C.jj  2011-08-03 14:06:03.000000000 +0200
> +++ gcc/testsuite/g++.dg/ext/builtin-object-size3.C     2011-08-03 14:04:21.000000000 +0200
> @@ -0,0 +1,26 @@
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +void baz (int *, int *);
> +
> +#define MEMCPY(d,s,l) __builtin___memcpy_chk (d, s, l, __builtin_object_size (d, 0))
> +
> +int
> +foo ()
> +{
> +  int *p = new int;
> +  int *q = new int[4];
> +  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int));
> +  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int));
> +  baz (p, q);
> +}
> +
> +int
> +bar ()
> +{
> +  int *p = new int;
> +  int *q = new int[4];
> +  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int) + 1);          // { dg-warning "will always overflow destination buffer" }
> +  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int) + 1);      // { dg-warning "will always overflow destination buffer" }
> +  baz (p, q);
> +}
>
>        Jakub
>

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Paolo Bonzini]

On 08/03/2011 02:46 PM, Richard Guenther wrote:
> If that's reasonable then adding the malloc attribute should be, too.

Making aliasing stricter for -D_FORTIFY_SOURCE=2 sounds wrong though.

Paolo

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Richard Guenther]

On Wed, Aug 3, 2011 at 3:06 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 08/03/2011 02:46 PM, Richard Guenther wrote:
>>
>> If that's reasonable then adding the malloc attribute should be, too.
>
> Making aliasing stricter for -D_FORTIFY_SOURCE=2 sounds wrong though.

The patch unconditionally adds malloc_size.

Richard.

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Jakub Jelinek]

On Wed, Aug 03, 2011 at 03:55:39PM +0200, Richard Guenther wrote:
> On Wed, Aug 3, 2011 at 3:06 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> > On 08/03/2011 02:46 PM, Richard Guenther wrote:
> >>
> >> If that's reasonable then adding the malloc attribute should be, too.
> >
> > Making aliasing stricter for -D_FORTIFY_SOURCE=2 sounds wrong though.
> 
> The patch unconditionally adds malloc_size.

alloc_size, yeah, unconditionally, on the other side it is used just
by __builtin_object_size which is used (usually) only for
-D_FORTIFY_SOURCE*.

I wanted to separate alloc_size attribute issue from malloc (where I'd
surely prefer also adding it by default and having an option to disable it
for weird C++ sources), because with -D_FORTIFY_SOURCE* or
__builtin_object_size one has to request it specially and/or use extensions
and thus not be that tighly bound by the standard.

	Jakub

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Jason Merrill]

On 08/03/2011 08:46 AM, Richard Guenther wrote:
> If that's reasonable then adding the malloc attribute should be, too.
> Finally.  Please.  Doesn't C++0x maybe "fix" the issue we were
> discussing to death?

Nope, as far as I can tell nobody raised it with the committee.  I have now.

I think we ought to be able to assume that a program which accesses the 
allocated storage other than through the returned pointer has undefined 
behavior.  I think that would be enough for attribute malloc, and I 
don't think that would interfere with reasonable pool allocators.

Jason

[RFC PATCH] Add alloc_size attribute to the default operator new and operator new[] (take 2)

[Jakub Jelinek]

On Wed, Aug 03, 2011 at 02:31:17PM +0200, Jakub Jelinek wrote:
> 2011-08-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/49905
> 	* decl.c (cxx_init_decl_processing): Add alloc_size (1) attribute
> 	for operator new and operator new [].

Unfortunately the patch caused a bunch of regressions, apparently
attribs.c initializes itself only on the call to decl_attributes, if there
was none, the hash table wasn't initialized and type hashing would crash
when seeing a type with TYPE_ATTRIBUTES.

This version passed bootstrap/regtest on i686-linux.

As for the properties which the middle-end would like to assume or not
from the operator new/operator new[]:
1) for alloc_size it is whether the returned pointer has exactly the
requested bytes defined, i.e. can't return a buffer where only fewer bytes
are valid and it is invalid to access bytes beyond those that were requested
2) aliasing - is the returned buffer guaranteed not to alias any other
object the program may validly access?
3) side-effects - currently for malloc we assume it has no visible
side-effects other than allocating the memory (i.e. malloc internals are
treated as black box), I guess for user supplied operator new/operator
new[] we shouldn't assume it doesn't have other side-effects (thus e.g. we
shouldn't optimize it away, etc.)
Richard, any other properties we are interested in?

2011-08-03  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/49905
	* tree.h (init_attributes): New prototype.
	* attribs.c (init_attributes): No longer static.

	* decl.c (cxx_init_decl_processing): Add alloc_size (1) attribute
	for operator new and operator new [].  Call init_attributes.

	* g++.dg/ext/builtin-object-size3.C: New test.

--- gcc/tree.h.jj	2011-08-02 18:08:11.000000000 +0200
+++ gcc/tree.h	2011-08-03 18:41:07.691420159 +0200
@@ -5547,6 +5547,8 @@ extern bool must_pass_in_stack_var_size_
 
 extern const struct attribute_spec *lookup_attribute_spec (const_tree);
 
+extern void init_attributes (void);
+
 /* Process the attributes listed in ATTRIBUTES and install them in *NODE,
    which is either a DECL (including a TYPE_DECL) or a TYPE.  If a DECL,
    it should be modified in place; if a TYPE, a copy should be created
--- gcc/attribs.c.jj	2011-06-22 12:32:22.000000000 +0200
+++ gcc/attribs.c	2011-08-03 17:57:43.977558394 +0200
@@ -34,8 +34,6 @@ along with GCC; see the file COPYING3.  
 #include "hashtab.h"
 #include "plugin.h"
 
-static void init_attributes (void);
-
 /* Table of the tables of attributes (common, language, format, machine)
    searched.  */
 static const struct attribute_spec *attribute_tables[4];
@@ -107,12 +105,15 @@ eq_attr (const void *p, const void *q)
 /* Initialize attribute tables, and make some sanity checks
    if --enable-checking.  */
 
-static void
+void
 init_attributes (void)
 {
   size_t i;
   int k;
 
+  if (attributes_initialized)
+    return;
+
   attribute_tables[0] = lang_hooks.common_attribute_table;
   attribute_tables[1] = lang_hooks.attribute_table;
   attribute_tables[2] = lang_hooks.format_attribute_table;
--- gcc/cp/decl.c.jj	2011-07-22 22:00:43.970484172 +0200
+++ gcc/cp/decl.c	2011-08-03 17:59:51.130796523 +0200
@@ -3629,6 +3629,7 @@ cxx_init_decl_processing (void)
   current_lang_name = lang_name_cplusplus;
 
   {
+    tree newattrs;
     tree newtype, deltype;
     tree ptr_ftype_sizetype;
     tree new_eh_spec;
@@ -3656,7 +3657,13 @@ cxx_init_decl_processing (void)
     else
       new_eh_spec = noexcept_false_spec;
 
-    newtype = build_exception_variant (ptr_ftype_sizetype, new_eh_spec);
+    /* Ensure attribs.c is initialized.  */
+    init_attributes ();
+    newattrs
+      = build_tree_list (get_identifier ("alloc_size"),
+			 build_tree_list (NULL_TREE, integer_one_node));
+    newtype = cp_build_type_attribute_variant (ptr_ftype_sizetype, newattrs);
+    newtype = build_exception_variant (newtype, new_eh_spec);
     deltype = build_exception_variant (void_ftype_ptr, empty_except_spec);
     push_cp_library_fn (NEW_EXPR, newtype);
     push_cp_library_fn (VEC_NEW_EXPR, newtype);
--- gcc/testsuite/g++.dg/ext/builtin-object-size3.C.jj	2011-08-03 16:08:07.732671633 +0200
+++ gcc/testsuite/g++.dg/ext/builtin-object-size3.C	2011-08-03 16:08:07.732671633 +0200
@@ -0,0 +1,26 @@
+// { dg-do compile }
+// { dg-options "-O2" }
+
+void baz (int *, int *);
+
+#define MEMCPY(d,s,l) __builtin___memcpy_chk (d, s, l, __builtin_object_size (d, 0))
+
+int
+foo ()
+{
+  int *p = new int;
+  int *q = new int[4];
+  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int));
+  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int));
+  baz (p, q);
+}
+
+int
+bar ()
+{
+  int *p = new int;
+  int *q = new int[4];
+  MEMCPY (p, "abcdefghijklmnopqrstuvwxyz", sizeof (int) + 1);		// { dg-warning "will always overflow destination buffer" }
+  MEMCPY (q, "abcdefghijklmnopqrstuvwxyz", 4 * sizeof (int) + 1);	// { dg-warning "will always overflow destination buffer" }
+  baz (p, q);
+}


	Jakub

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[] (take 2)

[Jason Merrill]

On 08/03/2011 02:26 PM, Jakub Jelinek wrote:
> As for the properties which the middle-end would like to assume or not
> from the operator new/operator new[]:
> 1) for alloc_size it is whether the returned pointer has exactly the
> requested bytes defined, i.e. can't return a buffer where only fewer bytes
> are valid and it is invalid to access bytes beyond those that were requested

"If it is successful, it shall return the address of the start of a 
block of storage whose length in bytes shall be at least as large as
the requested size."

I suppose this leaves room for a user operator new to allocate some 
extra space at the end and other code to take advantage of that, though 
I would be surprised if anyone actually did that.

> 2) aliasing - is the returned buffer guaranteed not to alias any other
> object the program may validly access?

Not currently.

> 3) side-effects - currently for malloc we assume it has no visible
> side-effects other than allocating the memory (i.e. malloc internals are
> treated as black box), I guess for user supplied operator new/operator
> new[] we shouldn't assume it doesn't have other side-effects (thus e.g. we
> shouldn't optimize it away, etc.)

We have no guarantees about what side-effects a user-supplied operator 
new might have.

Jason

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Gabriel Dos Reis]

On Wed, Aug 3, 2011 at 1:14 PM, Jason Merrill <jason@redhat.com> wrote:
> On 08/03/2011 08:46 AM, Richard Guenther wrote:
>>
>> If that's reasonable then adding the malloc attribute should be, too.
>> Finally.  Please.  Doesn't C++0x maybe "fix" the issue we were
>> discussing to death?
>
> Nope, as far as I can tell nobody raised it with the committee.  I have now.
>
> I think we ought to be able to assume that a program which accesses the
> allocated storage other than through the returned pointer has undefined
> behavior.

Hmm, how do you define "other than the returned pointer"?  Do you intend
to rule out garbage collectors?  Should not access as raw memory (e.g. through
char* or void*) be allowed?

> I think that would be enough for attribute malloc, and I don't
> think that would interfere with reasonable pool allocators.

I agree we ought to have a form of guarantee a la malloc attribute.

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Richard Guenther]

On Thu, Aug 4, 2011 at 2:58 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> On Wed, Aug 3, 2011 at 1:14 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 08/03/2011 08:46 AM, Richard Guenther wrote:
>>>
>>> If that's reasonable then adding the malloc attribute should be, too.
>>> Finally.  Please.  Doesn't C++0x maybe "fix" the issue we were
>>> discussing to death?
>>
>> Nope, as far as I can tell nobody raised it with the committee.  I have now.
>>
>> I think we ought to be able to assume that a program which accesses the
>> allocated storage other than through the returned pointer has undefined
>> behavior.
>
> Hmm, how do you define "other than the returned pointer"?  Do you intend
> to rule out garbage collectors?  Should not access as raw memory (e.g. through
> char* or void*) be allowed?

No.  But "other than the returned pointer" should probably
"other than through a pointer derived from the returned pointer".

>> I think that would be enough for attribute malloc, and I don't
>> think that would interfere with reasonable pool allocators.
>
> I agree we ought to have a form of guarantee a la malloc attribute.

Indeed.  Otherwise we depend too much on TBAA with all its C++ issues.

Richard.

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Richard Guenther]

On Thu, Aug 4, 2011 at 3:01 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 2:58 PM, Gabriel Dos Reis
> <gdr@integrable-solutions.net> wrote:
>> On Wed, Aug 3, 2011 at 1:14 PM, Jason Merrill <jason@redhat.com> wrote:
>>> On 08/03/2011 08:46 AM, Richard Guenther wrote:
>>>>
>>>> If that's reasonable then adding the malloc attribute should be, too.
>>>> Finally.  Please.  Doesn't C++0x maybe "fix" the issue we were
>>>> discussing to death?
>>>
>>> Nope, as far as I can tell nobody raised it with the committee.  I have now.
>>>
>>> I think we ought to be able to assume that a program which accesses the
>>> allocated storage other than through the returned pointer has undefined
>>> behavior.
>>
>> Hmm, how do you define "other than the returned pointer"?  Do you intend
>> to rule out garbage collectors?  Should not access as raw memory (e.g. through
>> char* or void*) be allowed?
>
> No.  But "other than the returned pointer" should probably
> "other than through a pointer derived from the returned pointer".

To make the point clearer, consider a C malloc implementation that
sets a global pointer to the last pointer it returned.  We "miscompile" then

extern int *last_malloc_result;
int main()
{
  int *p = malloc (4);
  *p = 0;
  *last_malloc_result = 1;
  return *p;
}

if malloc is declared with the malloc attribute.  Similar issues I can
see happening with C++ - but it's nothing special with C++ but
happens with C as well (given glibc malloc surely exposes interfaces
to get access to its pools behind our back).

Richard.

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Jason Merrill]

On 08/04/2011 08:58 AM, Gabriel Dos Reis wrote:
>Do you intend to rule out garbage collectors?

No, I suppose the rule should be that interleaved access through the 
returned pointer and other ways is undefined.

> Should not access as raw memory (e.g. through char* or void*) be allowed?

No, accessing it as raw memory is no different.

Jason

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Gabriel Dos Reis]

On Thu, Aug 4, 2011 at 8:43 AM, Jason Merrill <jason@redhat.com> wrote:
> On 08/04/2011 08:58 AM, Gabriel Dos Reis wrote:
>>
>> Do you intend to rule out garbage collectors?
>
> No, I suppose the rule should be that interleaved access through the
> returned pointer and other ways is undefined.

OK.

>> Should not access as raw memory (e.g. through char* or void*) be allowed?
>
> No, accessing it as raw memory is no different.

Hmm, maybe I misunderstand what you are saying.  But, I think a
scanning collector
should be allowed.

-- Gaby

Re: [RFC PATCH] Add alloc_size attribute to the default operator new and operator new[]

[Richard Guenther]

On Thu, Aug 4, 2011 at 3:50 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> On Thu, Aug 4, 2011 at 8:43 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 08/04/2011 08:58 AM, Gabriel Dos Reis wrote:
>>>
>>> Do you intend to rule out garbage collectors?
>>
>> No, I suppose the rule should be that interleaved access through the
>> returned pointer and other ways is undefined.
>
> OK.
>
>>> Should not access as raw memory (e.g. through char* or void*) be allowed?
>>
>> No, accessing it as raw memory is no different.
>
> Hmm, maybe I misunderstand what you are saying.  But, I think a
> scanning collector
> should be allowed.

But not interleaved with allocator users.  Problems will only arise if
you mix code using storage via the pointer returned from the allocator
and code that accesses the allocation pool by other means.
Where "mix" is, expose in one TU (actually expose partially, full
exposure is ok as well).

Richard.

> -- Gaby
>