[cfe-dev] RFC: Source-level attribute for conservative __builtin_object_size(..., 1)

[Hal Finkel via cfe-dev]

----- Original Message -----
> From: "Hal Finkel via cfe-dev" <cfe-dev at lists.llvm.org>
> To: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> Cc: "Richard Smith" <richard at metafoo.co.uk>, "CFE Dev" <cfe-dev at lists.llvm.org>, "George Burgess IV via llvm-commits"
> <llvm-commits at lists.llvm.org>
> Sent: Monday, March 7, 2016 6:00:16 PM
> Subject: Re: [cfe-dev] RFC: Source-level attribute for conservative __builtin_object_size(..., 1)
> 
> ----- Original Message -----
> > From: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: "Bob Wilson" <bob.wilson at apple.com>, "Richard Smith"
> > <richard at metafoo.co.uk>, "CFE Dev" <cfe-dev at lists.llvm.org>,
> > "George Burgess IV via llvm-commits" <llvm-commits at lists.llvm.org>
> > Sent: Monday, March 7, 2016 5:57:44 PM
> > Subject: Re: [cfe-dev] RFC: Source-level attribute for conservative
> > __builtin_object_size(..., 1)
> > 
> > 
> > > On 2016-Mar-07, at 15:07, Hal Finkel <hfinkel at anl.gov> wrote:
> > > 
> > > ----- Original Message -----
> > >> From: "Bob Wilson via cfe-dev" <cfe-dev at lists.llvm.org>
> > >> To: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> > >> Cc: "Richard Smith" <richard at metafoo.co.uk>, "CFE Dev"
> > >> <cfe-dev at lists.llvm.org>, "George Burgess IV via llvm-commits"
> > >> <llvm-commits at lists.llvm.org>
> > >> Sent: Monday, March 7, 2016 4:30:57 PM
> > >> Subject: Re: [cfe-dev] RFC: Source-level attribute for
> > >> conservative __builtin_object_size(..., 1)
> > >> 
> > >> 
> > >>> On Feb 25, 2016, at 12:43 PM, Duncan P. N. Exon Smith
> > >>> <dexonsmith at apple.com> wrote:
> > >>> 
> > >>> Context
> > >>> =======
> > >>> 
> > >>> r245403 and follow-ups improved __builtin_object_size(ptr, 1).
> > >>>  The
> > >>> type=1
> > >>> version of __builtin_object_size returns the maximum possible
> > >>> distance
> > >>> between `ptr` and the end of the subobject it's part of.  E.g.,
> > >>> in:
> > >>> ```
> > >>> struct S {
> > >>>  int i[2];
> > >>>  char c[5];
> > >>> };
> > >>> ```
> > >>> then __builtin_object_size(s.i, 1) should return 8, and
> > >>> __builtin_object_size(s.c + 2, 1) should return 3.
> > >>> 
> > >>> r250488 relaxed the rules for 0- or 1-sized arrays.  Consider
> > >>> ```
> > >>> struct S {
> > >>>  int i[2];
> > >>>  char c[0];
> > >>> };
> > >>> ```
> > >>> The expectation is that S will be over-allocated by some number
> > >>> of
> > >>> bytes,
> > >>> so that `S::c` has a runtime-determined array size.  r250488
> > >>> changed
> > >>> __builtin_object_size(ptr, 1) to be more conservative in this
> > >>> case,
> > >>> falling
> > >>> back to "type=0".  (Type=0 returns the maximum possible
> > >>> distance
> > >>> between
> > >>> `ptr` and the end of the allocation.)
> > >>> 
> > >>> Problem
> > >>> =======
> > >>> 
> > >>> We use __builtin_object_size(ptr, 1) in our strcpy
> > >>> implementation
> > >>> when
> > >>> _FORTIFY_SOURCE=2 (the default).  The code is something
> > >>> equivalent
> > >>> to:
> > >>> ```
> > >>> #define strcpy(a, b, sz) \
> > >>>  __builtin_strcpy_chk(a, b, sz, __builtin_object_size(a, 1))
> > >>> ```
> > >>> 
> > >>> This is causing a problem for some
> > >>> structs in our headers that end with a char array that has a
> > >>> size
> > >>> >
> > >>> 1 and
> > >>> that are expected to be over-allocated.
> > >>> 
> > >>> One example is `sockaddr_un`.  From the unix(4) manpage:
> > >>> http://www.unix.com/man-page/FreeBSD/4/unix/
> > >>> ```
> > >>> struct sockaddr_un {
> > >>>  u_char  sun_len;
> > >>>  u_char  sun_family;
> > >>>  char    sun_path[104];
> > >>> };
> > >>> ```
> > >>> 
> > >>> This has been around a long time.  Unfortunately,
> > >>> `sizeof(sockaddr_un)`
> > >>> cannot really change, and there's a ton of code out there that
> > >>> uses
> > >>> strcpy() on this struct.  We need some way for clang to be
> > >>> conservative
> > >>> in this case.
> > >>> 
> > >>> Solution
> > >>> ========
> > >>> 
> > >>> We're thinking about adding an attribute, something like:
> > >>> ```
> > >>> struct sockaddr_un {
> > >>>  u_char  sun_len;
> > >>>  u_char  sun_family;
> > >>>  char    sun_path[104] __attribute__((variable_length_array));
> > >>> };
> > >>> ```
> > >>> (is there a better name?).  This would allow us to decorate our
> > >>> headers,
> > >>> and explicitly opt-in on a struct-by-struct basis to the
> > >>> conservative
> > >>> behaviour that r250488 gives to 0- and 1-sized arrays.
> > >>> 
> > >>> The only other option we can think of us to be conservative
> > >>> whenever the
> > >>> the subobject is part of an array at the end of a struct.
> > >>> 
> > >>> Thoughts?
> > >> 
> > >> Ping. Does anyone have feedback on this?
> > > 
> > > If you can add an attribute to your headers, this seems like a
> > > good
> > > solution. We might also need the conservative option for dealing
> > > with older headers. Regarding the attribute, is is really a
> > > property of the array member, or a property of the structure
> > > (i.e.
> > > that it will be overallocated)?
> > 
> > I was thinking only of the narrow fix.  But applying it to the
> > struct
> > seems more interesting.  It formalizes other types of
> > over-allocation.
> > 
> > Also (maybe this is where you were going), this would let someone
> > add
> > the attribute after #include.
> > --
> > // Define 'struct something' in a header that can't be changed.
> > #include <something.h>
> > 
> > // Add an attribute.
> > struct something __attribute__((overallocated));
> > --
> > 
> > I like this better.
> 
> Great :-)
> 
> > 
> > > FWIW, the proposed attribute seems independently useful for
> > > eliminating unnecessary warnings. For example, if we have:
> > > 
> > > void foo(struct sockaddr_un *s) {
> > >  s->sun_path[106] = 0;
> > > }
> > > 
> > > we generate this:
> > > 
> > > warning: array index 106 is past the end of the array (which
> > > contains 104 elements) [-Warray-bounds]
> > >  s->sun_path[106] = 0;
> > > 
> > > which, as you describe the situation, is not useful.
> > 
> > Good point.  I think we could silence this with either attribute
> > location, right?
> 
> Yes, I think so.

Also, if we put this on classes, we could have a warning to catch the bugs Richard just fixed in r262891.

 -Hal

> 
>  -Hal
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory