Message ID | 20110803123117.GG2687@tyan-ft48-01.lab.bos.redhat.com |
---|---|
State | New |
Headers | show |
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 >
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
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.
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
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
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.
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.
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.
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
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
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 >
--- 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); +}