Re: [PATCH 2/2] Documentation Update.

[Kees Cook <keescook@chromium.org>]

On Thu, Feb 02, 2023 at 02:31:53PM +0000, Qing Zhao wrote:
> 
> > On Feb 2, 2023, at 3:33 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Wed, 1 Feb 2023, Siddhesh Poyarekar wrote:
> > 
> >> On 2023-02-01 13:24, Qing Zhao wrote:
> >>> 
> >>> 
> >>>> On Feb 1, 2023, at 11:55 AM, Siddhesh Poyarekar <siddhesh@gotplt.org>
> >>>> wrote:
> >>>> 
> >>>> On 2023-01-31 09:11, Qing Zhao wrote:
> >>>>> Update documentation to clarify a GCC extension on structure with
> >>>>> flexible array member being nested in another structure.
> >>>>> gcc/ChangeLog:
> >>>>> * doc/extend.texi: Document GCC extension on a structure containing
> >>>>> a flexible array member to be a member of another structure.
> >>>> 
> >>>> Should this resolve pr#77650 since the proposed action there appears to be
> >>>> to document these semantics?
> >>> 
> >>> My understanding of pr77650 is specifically for documentation on the
> >>> following case:
> >>> 
> >>> The structure with a flexible array member is the middle field of another
> >>> structure.
> >>> 
> >>> Which I added in the documentation as the 2nd situation.
> >>> However, I am still not very comfortable on my current clarification on this
> >>> situation: how should we document on
> >>> the expected gcc behavior to handle such situation?
> >> 
> >> I reckon wording that dissuades programmers from using this might be
> >> appropriate, i.e. don't rely on this and if you already have such nested flex
> >> arrays, change code to remove them.
> >> 
> >>>>> +In the above, @code{flex_data.data[]} is allowed to be extended flexibly
> >>>>> to
> >>>>> +the padding. E.g, up to 4 elements.
> >> 
> >> """
> >> ... Relying on space in struct padding is bad programming practice and any
> >> code relying on this behaviour should be modified to ensure that flexible
> >> array members only end up at the ends of arrays.  The `-pedantic` flag should
> >> help identify such uses.
> >> """
> >> 
> >> Although -pedantic will also flag on flex arrays nested in structs even if
> >> they're at the end of the parent struct, so my suggestion on the warning is
> >> not really perfect.
> > 
> > Wow, so I checked and we indeed accept
> > 
> > struct X { int n; int data[]; };
> > struct Y { struct X x; int end; };
> > 
> > and -pedantic says
> > 
> > t.c:2:21: warning: invalid use of structure with flexible array member 
> > [-Wpedantic]
> >    2 | struct Y { struct X x; int end; };
> >      |    
> 
> Currently, -pedantic report the same message for flex arrays nested in structs at the end of the parent struct AND in the middle of the parent struct. 
> Shall we distinguish them and report different warning messages in order to discourage the latter case? 
> 
> And at the same time, in the documentation, clarify these two situations, and discourage the latter case at the same time as well?
> >       
> > 
> > and clang reports
> > 
> > t.c:2:21: warning: field 'x' with variable sized type 'struct X' not at 
> > the end of a struct or class is a GNU extension 
> > [-Wgnu-variable-sized-type-not-at-end]
> > struct Y { struct X x; int end; };
> >  
> >                  ^
> 
> Clang’s warning message is clearer. 
> > 
> > looking at PR77650 what seems missing there is the semantics of this
> > extension as expected/required by the glibc use.  comment#5 seems
> > to suggest that for my example above its expected that
> > Y.x.data[0] aliases Y.end?!
> 
> Should we mentioned this alias relationship in the doc?
> 
> >  There must be a better way to write
> > the glibc code and IMHO it would be best to deprecate this extension.
> 
> Agreed. This is really a bad practice, should be deprecated. 
> We can give warning first in this release, and then deprecate this extension in a latter release. 

Right -- this can lead (at least) to type confusion and other problems
too. We've been trying to remove all of these overlaps in the Linux
kernel. I mention it the "Overlapping composite structure members"
section at https://people.kernel.org/kees/bounded-flexible-arrays-in-c

-- 
Kees Cook