diff mbox

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

Message ID 20110803123117.GG2687@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Aug. 3, 2011, 12:31 p.m. UTC 
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.


	Jakub

Comments

Richard Biener Aug. 3, 2011, 12:46 p.m. UTC | #1 
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
>
Paolo Bonzini Aug. 3, 2011, 1:06 p.m. UTC | #2 
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
Richard Biener Aug. 3, 2011, 1:55 p.m. UTC | #3 
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.
Jakub Jelinek Aug. 3, 2011, 2 p.m. UTC | #4 
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
Jason Merrill Aug. 3, 2011, 6:14 p.m. UTC | #5 
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
Gabriel Dos Reis Aug. 4, 2011, 12:58 p.m. UTC | #6 
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.
Richard Biener Aug. 4, 2011, 1:01 p.m. UTC | #7 
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.
Richard Biener Aug. 4, 2011, 1:18 p.m. UTC | #8 
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.
Jason Merrill Aug. 4, 2011, 1:43 p.m. UTC | #9 
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
Gabriel Dos Reis Aug. 4, 2011, 1:50 p.m. UTC | #10 
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
Richard Biener Aug. 4, 2011, 1:54 p.m. UTC | #11 
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
>
diff mbox

Patch

--- 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);
+}