Re: [GCC13][Patch][V2][1/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836

[Richard Biener <rguenther@suse.de>]

On Thu, 28 Jul 2022, Kees Cook wrote:

> On Thu, Jul 28, 2022 at 07:26:57AM +0000, Richard Biener wrote:
> > On Tue, 19 Jul 2022, Qing Zhao wrote:
> > > [...]
> > > +@cindex @code{strict_flex_array} variable attribute
> > > +@item strict_flex_array (@var{level})
> > > +The @code{strict_flex_array} attribute should be attached to the trailing
> > > +array field of a structure.  It specifies the level of strictness of
> > > +treating the trailing array field of a structure as a flexible array
> > > +member. @var{level} must be an integer betwen 0 to 3.
> > > +
> > > +@var{level}=0 is the least strict level, all trailing arrays of structures
> > > +are treated as flexible array members. @var{level}=3 is the strictest level,
> > > +only when the trailing array is declared as a flexible array member per C99
> > > +standard onwards ([]), it is treated as a flexible array member.
> > 
> > How is level 3 (thus -fstrict-flex-array) interpreted when you specify 
> > -std=c89?  How for -std=gnu89?
> 
> To me, it makes sense that either c99 is required (most sane to me)
> or it would disable flexible arrays entirely (seems an unlikely combo to
> be useful).
> 
> > 
> > > +
> > > +There are two more levels in between 0 and 3, which are provided to support
> > > +older codes that use GCC zero-length array extension ([0]) or one-size array
> > > +as flexible array member ([1]):
> > > +When @var{level} is 1, the trailing array is treated as a flexible array member
> > > +when it is declared as either "[]", "[0]", or "[1]";
> > > +When @var{level} is 2, the trailing array is treated as a flexible array member
> > > +when it is declared as either "[]", or "[0]".
> > 
> > Given the above does adding level 2 make sense given that [0] is a GNU
> > extension?
> 
> Level 1 removes the general "all trailing arrays are flex arrays" logic, but
> allows the 2 common "historical" fake flex array styles ("[1]" and "[0]").
> Level 2 additionally removes the "[1]" style.
> Level 3 additionally removes the "[0]" style.
> 
> I don't understand how "[0]" being a GNU extension matters here for
> level 2 -- it's dropping "[1]". And for level 3, the point is to defang
> the GNU extension of "[0]" to no longer mean "flexible array", and
> instead only mean "zero sized member" (as if it were something like
> "struct { } no_size;").
> 
> Note that for the Linux kernel, we only care about level 3, but could
> make do with level 2. We need to purge all the "fake" flexible array usage
> so we can start building a sane set of behaviors around array bounds
> that are reliably introspectable.

Note we've seen "historical" fake flex arrays like
struct X { int n; char str[4]; }; used by people being extra clever
(or careful?  char str[1] would not be a flex array since there's
a padding "member" behind it?!) in handling the padding.

I was just worried in confusing people too much.  Given 
-fstrict-flex-arrays enables level 3 should we warn with -std=c89
that it disables all flex arrays?  I think it should at least be
documented somehow.

> As a related bit of feature creep, it would be great to expose something
> like __builtin_has_flex_array_p() so FORTIFY could do a better job
> filtering __builtin_object_size() information.
> 
> Given:
> 
> struct inside {
>         int foo;
>         int bar;
>         unsigned long items[];
> };
> 
> struct outside {
>         int a;
>         int b;
>         struct inside inner;
> };
> 
> The follow properties are seen within, for example:
> 
> void stuff(struct outside *outer, struct inside *inner)
> {
> 	...
> }
> 
> 	__builtin_object_size(&outer->inner, 1) == 8
> 	__builtin_object_size(inner, 1)         == -1
> 
> (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832)

I think that would be a bug in bos worth fixing.

> So things like FORTIFY misfire on &outer->inner, as it's _not_ actually
> 8 bytes -- it has a potential trailing flex array.
> 
> If it could be introspected better, FORTIFY could check for the flex
> array. For example, instead of using the inconsistent __bos(ptr, 1) for
> finding member sizes, it could do something like:
> 
> #define __member_size(ptr)				\
> 	(__builtin_has_flex_array_p(ptr) ? -1 :		\
> 	 __builtin_object_size(ptr, 1))
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)