Bug 101836

Summary: __builtin_object_size(P->M, 1) where M is an array and the last member of a struct fails
Product: gcc Reporter: Kees Cook <kees>
Component: middle-endAssignee: qinzhao
Status: RESOLVED FIXED    
Severity: enhancement CC: crrodriguez, foom, hoffbrinkle, isanbard, jakub, jengelh, marxin, msebor, ndesaulniers, pageexec, qinzhao, siddhesh, sjames, toolchain
Priority: P3    
Version: 12.0   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2021-10-13 00:00:00
Attachments: struct layout changes bos1 behavior

Description Kees Cook 2021-08-09 23:25:41 UTC
Created attachment 51282 [details]
struct layout changes bos1 behavior

When bos1 is used on a member who is both an array and at the end of a structure, it fails to correctly resolve. This kind of behavior should only happen for flexible array members:

struct trailing_array {
    int a;
    int b;
    unsigned char c[16];
};

struct middle_array {
    int a;
    unsigned char c[16];
    int b;
};

ok:  sizeof(*working) == 24
ok:  sizeof(working->c) == 16
ok:  __builtin_object_size(working, 1) == -1
ok:  __builtin_object_size(working->c, 1) == 16
ok:  sizeof(*broken) == 24
ok:  sizeof(broken->c) == 16
ok:  __builtin_object_size(broken, 1) == -1
WAT: __builtin_object_size(broken->c, 1) == -1 (expected 16)
Comment 1 Andrew Pinski 2021-08-09 23:34:19 UTC
GCC treats all trailing arrays no matter what their size as flexible sized arrays.  This is by design because of many code out there assumes that.
Comment 2 Andrew Pinski 2021-08-09 23:35:15 UTC
See PR 44386.
Comment 3 Kees Cook 2021-08-10 01:19:27 UTC
Eww. That means _FORTIFY_SOURCE doesn't work correctly.

Can there please be a -fstrict-flex-arrays or something to turn off all the heuristics so a code base can declare it only uses flex arrays for dynamic trailing objects?
Comment 4 Martin Sebor 2021-10-13 02:11:00 UTC
I'm not sure how feasible it is to change __builtin_object_size or to add an option to control this behavior but I agree that treating all trailing arrays as flexible array members is overly permissive and unhelpful (GCC warnings like -Warray-bounds are stricter and treat only zero and one-element arrays that way).  Let me confirm this request and CC Siddhesh who just submitted a patch for __builtin_dynamic_object_size.  Maybe that's a way toward something stricter.
Comment 5 Siddhesh Poyarekar 2021-10-13 03:11:11 UTC
Vim explicitly disables _FORTIFY_SOURCE to keep its use of 1-sized trailing arrays:

https://github.com/vim/vim/issues/5581

so either they haven't tested with more recent gcc or they're hitting a corner case where __builtin_object_size does return the subscript value for the trailing member.

I inherited the __builtin_object_size behaviour in __builtin_dynamic_object_size to remain consistent with current behaviour:

ok:  sizeof(*working) == 24
ok:  sizeof(working->c) == 16
ok:  __builtin_object_size(working, 1) == -1
ok:  __builtin_object_size(working->c, 1) == 16
ok:  __builtin_dynamic_object_size(working, 1) == -1
ok:  __builtin_dynamic_object_size(working->c, 1) == 16
ok:  sizeof(*broken) == 24
ok:  sizeof(broken->c) == 16
ok:  __builtin_object_size(broken, 1) == -1
WAT: __builtin_object_size(broken->c, 1) == -1 (expected 16)
ok:  __builtin_dynamic_object_size(broken, 1) == -1
WAT: __builtin_dynamic_object_size(broken->c, 1) == -1 (expected 16)


However in theory if the pass can see the allocation, it should be able to build the right expression for object size.

I'm updating the patchset to meld the two passes into one and I could add -fstrict-flex-arrays as one of the patches to make this stricter.
Comment 6 qinzhao 2022-05-26 16:34:56 UTC
(In reply to Siddhesh Poyarekar from comment #5)
> I'm updating the patchset to meld the two passes into one and I could add
> -fstrict-flex-arrays as one of the patches to make this stricter.
The work for __builtin_dynamic_object_size seems has been committed into upstream gcc already, however without -fstrict-flex-arrays. 
do you have plan to add this new option into gcc in stage1?
Comment 7 Siddhesh Poyarekar 2022-05-27 01:45:31 UTC
I couldn't work on -fstrict-flex-arrays then, sorry.  I do have it in my plan for gcc 13, but I'll admit it's not on the very top of my list of things to do this year.  If you or anyone else needs a stronger guarantee of this making it into gcc 13 and wants to work on it, I'll be happy to help with reviews.
Comment 8 qinzhao 2022-05-27 14:27:24 UTC
(In reply to Siddhesh Poyarekar from comment #7)
> I couldn't work on -fstrict-flex-arrays then, sorry.  I do have it in my
> plan for gcc 13, but I'll admit it's not on the very top of my list of
> things to do this year.  If you or anyone else needs a stronger guarantee of
> this making it into gcc 13 and wants to work on it, I'll be happy to help
> with reviews.
thanks for the info.
will study this a little bit more and hopefully make it into gcc13.
Comment 9 Kees Cook 2022-05-27 21:01:04 UTC
Just to clarify, __builtin_dynamic_object_size() shouldn't have anything to do with this. What's needed is something like -fstrict-flex-arrays so that all the "trailing array is a flex array" assumptions can be killed everywhere in GCC. Only an _actual_ flex array should be treated as such.
Comment 10 Kees Cook 2022-05-27 21:08:18 UTC
Here's a slightly reworked example:
https://godbolt.org/z/EvehMax84
Comment 11 Kees Cook 2022-05-27 21:15:00 UTC
and with a flex array to compare:
https://godbolt.org/z/s9nb4Y7q4
Comment 12 qinzhao 2022-06-08 14:09:50 UTC
In the current tree-object-size.cc, "addr_object_size", it's clearly state the following:

 607               /* For &X->fld, compute object size only if fld isn't the last
 608                  field, as struct { int i; char c[1]; } is often used instead
 609                  of flexible array member.  */

and these part of codes were added back to 2009 with commit eb9ed98a951531f7fc40c69883b3285d58b168b2.

it's reasonable to add a new option -fstrict-flex-arrays to remove the "trailing array is a flex array" assumptions in current GCC. 

and the following utility routine that is added in tree.[h|cc] in 2020 can be used to identify whether a trailing array member reference is a flexible array or not:


/* Describes a "special" array member due to which component_ref_size
   returns null.  */
enum struct special_array_member
  {
   none,      /* Not a special array member.  */
   int_0,     /* Interior array member with size zero.  */
   trail_0,   /* Trailing array member with size zero.  */
   trail_1    /* Trailing array member with one element.  */
  };


/* Determines the size of the member referenced by the COMPONENT_REF
   REF, using its initializer expression if necessary in order to
   determine the size of an initialized flexible array member.
   If non-null, set *ARK when REF refers to an interior zero-length
   array or a trailing one-element array.
   Returns the size as sizetype (which might be zero for an object
   with an uninitialized flexible array member) or null if the size
   cannot be determined.  */

tree
component_ref_size (tree ref, special_array_member *sam /* = NULL */)
Comment 13 Kees Cook 2022-06-08 15:37:08 UTC
Maybe the enum needs to also be expanded so that [0] can be distinguished from []?
Comment 14 qinzhao 2022-06-08 16:06:47 UTC
(In reply to Kees Cook from comment #13)
> Maybe the enum needs to also be expanded so that [0] can be distinguished
> from []?

I believe that the IR for real flexible array [] is different from [0], it's already identified by the IR itself, no need for this enum to distinguish.
Comment 15 qinzhao 2022-06-10 20:15:18 UTC
the following patch will fix the issue with this testing case:

[opc@qinzhao-ol8u3-x86 gcc]$ git diff
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 5ca87ae3504..7df092346b9 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -604,9 +604,8 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
          else if (var != pt_var && TREE_CODE (pt_var) == MEM_REF)
            {
              tree v = var;
-             /* For &X->fld, compute object size only if fld isn't the last
-                field, as struct { int i; char c[1]; } is often used instead
-                of flexible array member.  */
+             /* For &X->fld, compute object size if fld isn't a flexible array
+                member.  */
              while (v && v != pt_var)
                switch (TREE_CODE (v))
                  {
@@ -645,12 +644,19 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
                        && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
                           == RECORD_TYPE)
                      {
-                       tree fld_chain = DECL_CHAIN (TREE_OPERAND (v, 1));
-                       for (; fld_chain; fld_chain = DECL_CHAIN (fld_chain))
-                         if (TREE_CODE (fld_chain) == FIELD_DECL)
-                           break;
-
-                       if (fld_chain)
+                       bool is_flexible_array = false;
+                       /* Set for accesses to special trailing arrays.  */
+                       special_array_member sam{ };
+
+                       tree refsize = component_ref_size (v, &sam);
+                       /* if the array is a special trailing array, don't compute
+                        * its size, otherwise, treat it as a normal array.  */
+                       if (sam == special_array_member::trail_0
+                           || sam == special_array_member::trail_1
+                           || flexible_array_type_p (TREE_TYPE (TREE_OPERAND (v,0))))
+                         is_flexible_array = true;
+
+                       if (!is_flexible_array)
                          {
                            v = NULL_TREE;
Comment 16 qinzhao 2022-06-10 20:24:48 UTC
additional work are needed in order to make this task complete:

1. add one more new gcc option:

-fstrict-flex-arrays

when it's on, only treat the following cases as flexing array:

trailing array with size 0;
trailing array with size 1;
trailing flexible array;

all other trailing arrays with size > 1 will be treated as normal arrays. 


2. there a lot of places in GCC that currently assume all trailing arrays as flexible array, we might need to update all these places altogether to make GCC behavior consistently. 

As I checked, most of the places used an old routine array_at_struct_end_p, we might need to replace all the usage of "array_at_struct_end_p" with the new option + the more strict checking on flexing trailing array. 

let me know if you have any comments and suggestions.
Comment 17 Kees Cook 2022-06-11 08:21:58 UTC
(In reply to qinzhao from comment #16)
> additional work are needed in order to make this task complete:
> 
> 1. add one more new gcc option:
> 
> -fstrict-flex-arrays
> 
> when it's on, only treat the following cases as flexing array:
> 
> trailing array with size 0;
> trailing array with size 1;
> trailing flexible array;
> 
> all other trailing arrays with size > 1 will be treated as normal arrays. 

Under -fstrict-flex-arrays, arrays of size 0 and 1 should *not* be treated as flex arrays. Only "[]" should be a flexible array. Everything else should be treated as having the literal size given.
Comment 18 Martin Sebor 2022-06-13 14:48:17 UTC
The zero size case exists (and is documented) solely as a substitute for flexible array members.  Treating is as an ordinary array would disable that extension.  It might be appropriate to provide a separate option to control it but conflating it with the other cases (one or more elements) doesn't seem like the robust design.

As I mentioned in the review of the Clang change, https://reviews.llvm.org/D126864, so that code bases that use some larger number of elements than zero, such as one, and that can't easily change, can still benefit from the BOS enhancement for the remaining cases, it would be helpful for the new option to accept the minimum number of elements at which a trailing array ceases to be considered a poor-man's flexible array member.
Comment 19 Kees Cook 2022-06-13 18:12:09 UTC
(In reply to Martin Sebor from comment #18)
> The zero size case exists (and is documented) solely as a substitute for
> flexible array members.  Treating is as an ordinary array would disable that
> extension.  It might be appropriate to provide a separate option to control
> it but conflating it with the other cases (one or more elements) doesn't
> seem like the robust design.
> 
> As I mentioned in the review of the Clang change,
> https://reviews.llvm.org/D126864, so that code bases that use some larger
> number of elements than zero, such as one, and that can't easily change, can
> still benefit from the BOS enhancement for the remaining cases, it would be
> helpful for the new option to accept the minimum number of elements at which
> a trailing array ceases to be considered a poor-man's flexible array member.

I see your point about gaining the "trailing array" fix without breaking the older code bases, but that doesn't seem to fit the name (nor purpose) of -fstrict-flex-arrays, which should be considered a "complete" fix.

To me it looks like -fstrict-flex-arrays should kill the [0] extension, the ancient [1] misuse, and the "anything trailing is flex" logic. If fixing _only_ the latter is desired, perhaps add an option for that, but no one is actually asking for it yet. :) The Linux kernel wants the "fully correct" mode.
Comment 20 Martin Sebor 2022-06-13 22:27:43 UTC
Well, I just "asked" for such an option the same way you asked for -fstrict-flex-arrays in comment #3, because I believe it would be useful to make the BOS improvements you're looking for available even to code that can't do a whole-hog replacement of all trailing arrays with flexible array members.  The spelling of the option names doesn't seem important to me (they could be separate options, or the same one with an argument).
Comment 21 Kees Cook 2022-06-14 00:00:31 UTC
(In reply to Martin Sebor from comment #20)
> Well, I just "asked" for such an option the same way you asked for
> -fstrict-flex-arrays in comment #3, because I believe it would be useful to
> make the BOS improvements you're looking for available even to code that
> can't do a whole-hog replacement of all trailing arrays with flexible array

Right, sorry, I meant, "I have a project waiting to use this feature right now", where as other projects might, upon discovering this feature, decide they also only need "-fstrict-flex-arrays". e.g. what option would GCC itself use?

> members.  The spelling of the option names doesn't seem important to me
> (they could be separate options, or the same one with an argument).

How about "-fnot-flex-arrays=N" to mean "trailing arrays with N or more elements will NOT be treated like a flex array"?

Then code with sockaddr can use "-fnot-flex-arrays=15", code with "[1]" arrays can use "-fnot-flex-arrays=2", code with only "[0]" arrays can use "-fnot-flex-arrays=1", and "-fstrict-flex-arrays" can be an alias for "-fnot-flex-arrays=0", which Linux would use.
Comment 22 Siddhesh Poyarekar 2022-06-14 05:09:48 UTC
(In reply to Kees Cook from comment #21)
> How about "-fnot-flex-arrays=N" to mean "trailing arrays with N or more
> elements will NOT be treated like a flex array"?
> 
> Then code with sockaddr can use "-fnot-flex-arrays=15", code with "[1]"
> arrays can use "-fnot-flex-arrays=2", code with only "[0]" arrays can use
> "-fnot-flex-arrays=1", and "-fstrict-flex-arrays" can be an alias for
> "-fnot-flex-arrays=0", which Linux would use.

An arbitrary N will only make it abuse-friendly and potentially mask bugs.  IMO if we choose to make multiple levels here it should only be -fstrict-flex-arrays={1,2} where 1 (the default) only allows "[]" and 2 allows "[0]", disabling all other size values.  For anything else, -fno-strict-flex-arrays.  My opinion on the default is not strong FWIW.
Comment 23 Siddhesh Poyarekar 2022-06-14 05:21:46 UTC
(In reply to Siddhesh Poyarekar from comment #22)
> An arbitrary N will only make it abuse-friendly and potentially mask bugs. 
> IMO if we choose to make multiple levels here it should only be
> -fstrict-flex-arrays={1,2} where 1 (the default) only allows "[]" and 2
> allows "[0]", disabling all other size values.  For anything else,

That could be ""[0]" or "[1]", disabling all other size values" if we want to build gcc and vim with -fstrict-flex-arrays and keep fortification enabled.  Vim explicitly disables fortification right now for this reason.

> -fno-strict-flex-arrays.  My opinion on the default is not strong FWIW.

Also I wonder if there should be an analogous -Wstrict-flex-arrays to issue warnings alongside changing codegen.
Comment 24 Jakub Jelinek 2022-06-14 07:25:47 UTC
For the default, a complication is that standard C++ doesn't allow neither flexible array members nor zero sized arrays, so unless one uses extensions one can only write [1].
I think differentiating between only allowing [] as flex, or [] and [0],
or [], [0] and [1], or any trailing array is useful.
Comment 25 qinzhao 2022-06-14 15:00:09 UTC
So, based on all the discussion so far, how about the following:

** add the following gcc option:

-fstrict-flex-arrays=[0|1|2|3]

when -fstrict-flex-arrays=0:
treat all trailing arrays as flexible arrays. the default behavior;

when -fstrict-flex-arrays=1:
Only treating [], [0], and [1] as flexible array;

when -fstrict-flex-arrays=2:
Only treating [] and [0] as flexible array;

when -fstrict-flex-arrays=3:
Only treating [] as flexible array; The strictest level. 

any comments?
Comment 26 Siddhesh Poyarekar 2022-06-14 15:39:54 UTC
(In reply to qinzhao from comment #25)
> So, based on all the discussion so far, how about the following:
> 
> ** add the following gcc option:
> 
> -fstrict-flex-arrays=[0|1|2|3]
> 
> when -fstrict-flex-arrays=0:
> treat all trailing arrays as flexible arrays. the default behavior;

Wouldn't this be -fno-strict-flex-arrays, i.e. the current behaviour?

> when -fstrict-flex-arrays=1:
> Only treating [], [0], and [1] as flexible array;
> 
> when -fstrict-flex-arrays=2:
> Only treating [] and [0] as flexible array;
> 
> when -fstrict-flex-arrays=3:
> Only treating [] as flexible array; The strictest level.

If yes, then you end up having:

-fstrict-flex-arrays=[1|2|3]

with, I suppose, 1 as the default based on Jakub's comment about maximum compatibility support.
Comment 27 Qing Zhao 2022-06-14 16:02:44 UTC
> On Jun 14, 2022, at 11:39 AM, siddhesh at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org> wrote:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
> 
> --- Comment #26 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
> (In reply to qinzhao from comment #25)
>> So, based on all the discussion so far, how about the following:
>> 
>> ** add the following gcc option:
>> 
>> -fstrict-flex-arrays=[0|1|2|3]
>> 
>> when -fstrict-flex-arrays=0:
>> treat all trailing arrays as flexible arrays. the default behavior;
> 
> Wouldn't this be -fno-strict-flex-arrays, i.e. the current behaviour?

Yes, it’s the same.  =0 is aliased with -fno-strict-flex-arrays.

The point is, the larger the value of LEVEL, the stricter with treating the flexing array.

i.e, 0 is the least strict, and 3 is the strictest mode.

But we can delete the level 0 if not necessary.
> 
>> when -fstrict-flex-arrays=1:
>> Only treating [], [0], and [1] as flexible array;
>> 
>> when -fstrict-flex-arrays=2:
>> Only treating [] and [0] as flexible array;
>> 
>> when -fstrict-flex-arrays=3:
>> Only treating [] as flexible array; The strictest level.
> 
> If yes, then you end up having:
> 
> -fstrict-flex-arrays=[1|2|3]
> 
> with, I suppose, 1 as the default based on Jakub's comment about maximum
> compatibility support.
Yes.  And 3 is the one Kees requested for kernel usage.
Comment 28 Jakub Jelinek 2022-06-14 16:17:50 UTC
(In reply to Qing Zhao from comment #27)
> > Wouldn't this be -fno-strict-flex-arrays, i.e. the current behaviour?
> 
> Yes, it’s the same.  =0 is aliased with -fno-strict-flex-arrays.

That is indeed what we do for many options, -fno-whatever is alias to
-fwhatever=0 (or -fwhatever=something for options which take enums and not numbers).
Comment 29 qinzhao 2022-06-14 21:03:17 UTC
(In reply to Jakub Jelinek from comment #28)
> (In reply to Qing Zhao from comment #27)
> > > Wouldn't this be -fno-strict-flex-arrays, i.e. the current behaviour?
> > 
> > Yes, it’s the same.  =0 is aliased with -fno-strict-flex-arrays.
> 
> That is indeed what we do for many options, -fno-whatever is alias to
> -fwhatever=0 (or -fwhatever=something for options which take enums and not
> numbers).

thank you for the info.
could you point me an example of such option? then I can check to see how to implement this alias relationship between =0 and -fno-strict-flex-arrays?
Comment 30 Jakub Jelinek 2022-06-14 21:12:55 UTC
grep shows:
common.opt:Common Alias(Wattribute_alias=, 1, 0) Warning
common.opt:Common Alias(Wimplicit-fallthrough=,3,0) Warning
c-family/c.opt:C ObjC C++ ObjC++ Warning Alias(Warray-parameter=, 2, 0)
c-family/c.opt:C++ ObjC++ Warning Alias(Wcatch-value=, 1, 0)
c-family/c.opt:C ObjC C++ LTO ObjC++ Alias(Wdangling-pointer=, 2, 0) Warning
c-family/c.opt:C ObjC C++ ObjC++ Warning Alias(Wformat=, 1, 0)
c-family/c.opt:C ObjC C++ LTO ObjC++ Warning Alias(Wformat-overflow=, 1, 0) IntegerRange(0, 2)
c-family/c.opt:C ObjC C++ LTO ObjC++ Warning Alias(Wformat-truncation=, 1, 0)
c-family/c.opt:C ObjC C++ LTO ObjC++ Warning Alias(Wstringop-overflow=, 2, 0)
c-family/c.opt:C++ Warning Alias(Wplacement-new=, 1, 0)
c-family/c.opt:C ObjC C++ ObjC++ Warning Alias(Wshift-overflow=, 1, 0)
c-family/c.opt:C ObjC C++ ObjC++ Warning Alias(Wunused-const-variable=, 2, 0)
c-family/c.opt:C++ ObjC++ Alias(faligned-new=,1,0)
fortran/lang.opt:Fortran Alias(ftail-call-workaround=,1,0)
Comment 31 James Y Knight 2022-06-25 00:52:05 UTC
It doesn't make sense to have a mode in which `int array[0]` is accepted but is not a flex array.

Either that should be a compilation error (as the standard specifies), or it should be a flex array. Accepting it as an extension but having it do the wrong thing is not useful or helpful.

Note that Clang has a dedicated warning flag for zero-length arrays: -Wzero-length-array, so anyone who wants to prohibit them may use -Werror=zero-length-array. It would be helpful for GCC could follow suit there.

The other proposed modes:
- Treat all trailing arrays as flexible arrays. the default behavior;
- Only treating [], [0], and [1] as flexible array;
- Only treating [] and [0] as flexible array;
do make sense.
Comment 32 qinzhao 2022-06-27 14:01:42 UTC
(In reply to James Y Knight from comment #31)
> It doesn't make sense to have a mode in which `int array[0]` is accepted but
> is not a flex array.
> 
> Either that should be a compilation error (as the standard specifies), or it
> should be a flex array. Accepting it as an extension but having it do the
> wrong thing is not useful or helpful.
> 
> Note that Clang has a dedicated warning flag for zero-length arrays:
> -Wzero-length-array, so anyone who wants to prohibit them may use
> -Werror=zero-length-array. It would be helpful for GCC could follow suit
> there.

there is a Bugzilla that has been filed for GCC to request the same warning for GCC:
https://gcc.gnu.org/bugzilla//show_bug.cgi?id=94428

-Wzero-length-array


As suggested by Siddhesh in comment#23, -Wstrict-flex-arrays might be necessary to be added too, and 
-Wzero-length-array will be an alias to 
-Wstrict-flex-arrays=3
Comment 33 James Y Knight 2022-07-06 16:16:56 UTC
(In reply to qinzhao from comment #32)
> there is a Bugzilla that has been filed for GCC to request the same warning
> for GCC:
> https://gcc.gnu.org/bugzilla//show_bug.cgi?id=94428
> 
> -Wzero-length-array

Great. Adding that flag, and eliminating the -fstrict-flex-arrays=3 option from this proposal would be good.

> As suggested by Siddhesh in comment#23, -Wstrict-flex-arrays might be
> necessary to be added too, and 
> -Wzero-length-array will be an alias to 
> -Wstrict-flex-arrays=3

I don't understand what the -Wstrict-flex-arrays warning and its multiple levels is proposed to actually do.

Is it supposed to warn on the structs that change behavior in the corresponding -fstrict-flex-array=LEVEL? But that would mean -Wstrict-flex-arrays=1 would warn on any array at the end of a struct which has a size other than 0 or 1. That's clearly not going to be actually practical...so perhaps you had something else in mind?
Comment 34 Kees Cook 2022-07-06 16:49:46 UTC
-fstrict-flex-arrays=3 is still needed. (E.g. for proper FORTIFY coverage, etc.) I don't have an opinion about the -W options, though.(In reply to James Y Knight from comment #33)
> (In reply to qinzhao from comment #32)
> > there is a Bugzilla that has been filed for GCC to request the same warning
> > for GCC:
> > https://gcc.gnu.org/bugzilla//show_bug.cgi?id=94428
> > 
> > -Wzero-length-array
> 
> Great. Adding that flag, and eliminating the -fstrict-flex-arrays=3 option
> from this proposal would be good.

Hmm? No, -fstrict-flex-arrays=3 is still needed (because it changes compiler _behavior_, e.g. for proper FORTIFY coverage or trailing arrays, etc).

I don't have a strong opinion about the -W options; but they can't warn if they just see a struct declaration with a 0 or 1 element array: userspace will have those for years to come. Maybe it would warn if such a struct member is ever actually used in the code? That kind of behavior would be useful for the Linux kernel at least.
Comment 35 qinzhao 2022-07-06 17:53:30 UTC
(In reply to James Y Knight from comment #33)

> 
> I don't understand what the -Wstrict-flex-arrays warning and its multiple
> levels is proposed to actually do.
> 
> Is it supposed to warn on the structs that change behavior in the
> corresponding -fstrict-flex-array=LEVEL? But that would mean
> -Wstrict-flex-arrays=1 would warn on any array at the end of a struct which
> has a size other than 0 or 1. That's clearly not going to be actually
> practical...so perhaps you had something else in mind?


I think that -Wstrict-flex-arrays will need to be cooperated with -fstrict-flex-arrays=N, i.e, only when -fstrict-flex-arrays=N is presenting, -Wstrict-flex-arrays will be valid and report the warnings when see a [0], or [1], or any trailing array based on N:

when -fstrict-flex-arrays
=0, -Wstrict-flex-arrays will NOT issue any warnings;
=1, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array;
=2, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array or [1];
=3, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array or [1], or [0].

let me know if you have any comment and suggestion on this.
Comment 36 James Y Knight 2022-07-06 19:30:32 UTC
(In reply to Kees Cook from comment #34)
> > Great. Adding that flag, and eliminating the -fstrict-flex-arrays=3 option
> > from this proposal would be good.
> 
> Hmm? No, -fstrict-flex-arrays=3 is still needed (because it changes compiler
> _behavior_, e.g. for proper FORTIFY coverage or trailing arrays, etc).

There is no purpose served by writing a struct member `int x[0];` other than to create a FAM. Zero-length arrays are not permitted by the C standard, but are a GCC compiler extension explicitly for the purpose of creating a FAM. This is entirely unlike `int x[1];` or `int x[10];` which of course have a primary meaning as a concrete array size...

If the linux kernel doesn't want to allow `int x[0];` FAMs, then prohibit them entirely using -Werror=zero-length-array (once it's implemented).
Comment 37 James Y Knight 2022-07-06 19:46:59 UTC
(In reply to qinzhao from comment #35)
> I think that -Wstrict-flex-arrays will need to be cooperated with
> -fstrict-flex-arrays=N, i.e, only when -fstrict-flex-arrays=N is presenting,
> -Wstrict-flex-arrays will be valid and report the warnings when see a [0],
> or [1], or any trailing array based on N:

When -fstrict-flex-arrays is used, I'd expect the existing -Warray-bounds warning to already emit diagnostics in the cases you list, because those cases should no longer be special-cased to act as a FAM, and thus no longer explicitly suppress it.
Comment 38 qinzhao 2022-07-06 20:01:48 UTC
(In reply to James Y Knight from comment #37)
> (In reply to qinzhao from comment #35)
> > I think that -Wstrict-flex-arrays will need to be cooperated with
> > -fstrict-flex-arrays=N, i.e, only when -fstrict-flex-arrays=N is presenting,
> > -Wstrict-flex-arrays will be valid and report the warnings when see a [0],
> > or [1], or any trailing array based on N:
> 
> When -fstrict-flex-arrays is used, I'd expect the existing -Warray-bounds
> warning to already emit diagnostics in the cases you list, because those
> cases should no longer be special-cased to act as a FAM, and thus no longer
> explicitly suppress it.

yes, that's right. instead of adding a new -Wstrict-flex-arrays, we can also update the current -Warray-bounds to cooperate with the new -fstrict-flex-arrays to issue warnings according to the different level of strict-flex-arrays.
Comment 39 qinzhao 2022-07-06 20:05:09 UTC
(In reply to Siddhesh Poyarekar from comment #23)

> Also I wonder if there should be an analogous -Wstrict-flex-arrays to issue
> warnings alongside changing codegen.

please take a look at comments #32 and above to see whether the discussion of -Wstrict-flex-arrays is similar as what are in your mind?
Comment 40 Bill Wendling 2022-07-22 22:33:55 UTC
(In reply to James Y Knight from comment #36)
> (In reply to Kees Cook from comment #34)
> > > Great. Adding that flag, and eliminating the -fstrict-flex-arrays=3 option
> > > from this proposal would be good.
> > 
> > Hmm? No, -fstrict-flex-arrays=3 is still needed (because it changes compiler
> > _behavior_, e.g. for proper FORTIFY coverage or trailing arrays, etc).
> 
> There is no purpose served by writing a struct member `int x[0];` other than
> to create a FAM. Zero-length arrays are not permitted by the C standard, but
> are a GCC compiler extension explicitly for the purpose of creating a FAM.
> This is entirely unlike `int x[1];` or `int x[10];` which of course have a
> primary meaning as a concrete array size...
> 
> If the linux kernel doesn't want to allow `int x[0];` FAMs, then prohibit
> them entirely using -Werror=zero-length-array (once it's implemented).

[Kees, correct me if I'm wrong.]

I don't think this satisfies what Kees initially asked for. The GCC extension that a trailing `[0]' array in a structure is causing FORTIFY to fail. It would be great to remove them all, but that's more-or-less a separate issue from making FORTIFY work in all instances. (From what I understand, removing the trailing `[0]' from Linux is an ongoing project.)

The question then is if `-fstrict-flex-arrays=3' is used, what does a `[0]' at the end of a struct represent (assuming GCC no longer treats it as an FAM)?
Comment 41 Kees Cook 2022-07-23 01:57:42 UTC
(In reply to Bill Wendling from comment #40)
> The question then is if `-fstrict-flex-arrays=3' is used, what does a `[0]'
> at the end of a struct represent (assuming GCC no longer treats it as an
> FAM)?

It's a zero-sized object. The same as `struct { } foo;`
Comment 42 GCC Commits 2022-10-07 17:44:53 UTC
The master branch has been updated by Qing Zhao <qinzhao@gcc.gnu.org>:

https://gcc.gnu.org/g:b9ad850e86b863c24f6f4f5acf08d49944cc7bbe

commit r13-3171-gb9ad850e86b863c24f6f4f5acf08d49944cc7bbe
Author: Qing Zhao <qing.zhao@oracle.com>
Date:   Fri Oct 7 14:59:01 2022 +0000

    Use array_at_struct_end_p in __builtin_object_size [PR101836]
    
    Use array_at_struct_end_p to determine whether the trailing array
    of a structure is flexible array member in __builtin_object_size.
    
    gcc/ChangeLog:
    
            PR tree-optimization/101836
            * tree-object-size.cc (addr_object_size): Use array_at_struct_end_p
            to determine a flexible array member reference.
    
    gcc/testsuite/ChangeLog:
    
            PR tree-optimization/101836
            * gcc.dg/pr101836.c: New test.
            * gcc.dg/pr101836_1.c: New test.
            * gcc.dg/pr101836_2.c: New test.
            * gcc.dg/pr101836_3.c: New test.
            * gcc.dg/pr101836_4.c: New test.
            * gcc.dg/pr101836_5.c: New test.
            * gcc.dg/strict-flex-array-2.c: New test.
            * gcc.dg/strict-flex-array-3.c: New test.
Comment 43 qinzhao 2022-12-21 20:47:17 UTC
the related patch list for this work is (gcc13)

2a27ae32fabf85685ffff758459d7ec284ccb95a
710c9676520dfd38b4bfdcc937ce026ed89921d6
ace0ae09332bbc6b95e084c2c2b17c466339ff76
b9ad850e86b863c24f6f4f5acf08d49944cc7bbe
1879e48f3d8595bc9e7f583bbd12df3c6f5c42dc
Comment 44 qinzhao 2023-06-21 19:47:32 UTC
fixed in gcc13 already