Re: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.

[Kees Cook <keescook@chromium.org>]

On Tue, May 07, 2024 at 06:34:19PM +0000, Qing Zhao wrote:
> On May 7, 2024, at 13:57, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
> > On 07.05.24 16:26, Qing Zhao wrote:
> > > Hi, Sebastian,
> > > Thanks for your explanation.
> > > Our goal is to deprecate the GCC extension on  structure
> > > containing a flexible array member not at the end of another
> > > structure. In order to achieve this goal, we provided the warning option
> > > -Wflex-array-member-not-at-end for the users to locate all such
> > > cases in their source code and update the source code to eliminate
> > > such cases.
> >
> > What is the benefit of deprecating this GCC extension? If GCC
> > extensions are removed, then it would be nice to enable the associated
> > warnings by default.

The goal of all of the recent array bounds and flexible array work is to
make sizing information unambiguous (e.g. via __builtin_object_size(),
__builtin_dynamic_object_size(), and the array-bounds sanitizer). For
the compiler to be able to deterministically report size information
on arrays, we needed to deprecate this case even though it had been
supported in the past. (Though we also _added_ extensions to support
for other things, like flexible arrays in unions, and the coming
__counted_by attribute.)

For example:

struct flex     { int length; char data[]; };
struct mid_flex { int m; struct flex flex_data; int n; int o; };

#define SZ(p)	__builtin_dynamic_object_size(p, 1)

void foo(struct flex *f, struct mid_flex *m)
{
	printf("%zu\n", SZ(f));
	printf("%zu\n", SZ(m->flex_data));
}

int main(void)
{
        struct mid_flex m = { .flex_data.length = 8 };
	foo(&m->flex_data, &m);
	return 0;
}

This is printing the size of the same object. But the desired results
are ambiguous. Does m->flex_data have an unknown size (i.e. SIZE_MAX)
because it's a flex array, or does it contain 8 bytes, since it overlaps
with the other structure's trailing 2 ints?

The answer from GCC 13 was neither:

18446744073709551615
4

It considered flex_data to be only the size of it's non-flex-array
members, but only when there was semantic context that it was part of
another structure. (Yet more ambiguity.)

In GCC 14, this is "resolved" to be unknown since it is a flex array
which has no sizing info, and context doesn't matter:

18446744073709551615
18446744073709551615

But this paves the way for the coming 'counted_by' attribute which will
allow for struct flex above to be defined as:

struct flex { int length; char data[] __attribute__((counted_by(length))); };

At which point GCC can deterministically report the object size.

Hopefully I've captured this all correctly -- Qing can correct me. :)

> >
> > > We had a long discussion before deciding to deprecating this GCC
> > > extension. Please see details here:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
> > >
> > > Yes, we do plan to enable this warning by default before final
> > > deprecation.  (Might consider to enable this warning by default in
> > > GCC15… and then deprecate it in the next release)
> > >
> > > Right now, there is an ongoing work in Linux kernel to get rid of
> > > all such cases. Kees might have more information on this.
> > >
> > >
> > > The static initialization of structures with flexible array members
> > > will still work as long as the flexible array members are at the end of
> > > the structures.
> >
> > Removing the support for flexible array members in the middle of
> > compounds will make the static initialization practically infeasible.
>
> If the flexible array member is moved to the end of the compounds,
> the static initialization still work. What’s the issue here?
>
> > > My question: is it possible to update your source code to move
> > > the structure with flexible array member to the end of the containing
> > > structure?
> > >
> > > i.e, in your example, in the struct Thread_Configured_control,
> > > move the field “Thread_Control Control” to the end of the structure?
> >
> > If we move the Thread_Control to the end, how would I add a
> > configuration defined number of elements at the end?
>
> Don’t understand this, why moving the Thread_Control Control” to
> the end of the containing structure will make this a problem?
> Could you please explain this with a simplified example?

I found your example at [2] and tried to trim/summarize it here:


struct _Thread_Control {
    Objects_Control Object;
    ...
    void            *extensions[];
};
typedef struct _Thread_Control Thread_Control;

struct Thread_Configured_control {
  Thread_Control Control;

  #if CONFIGURE_MAXIMUM_USER_EXTENSIONS > 0
    void *extensions[ CONFIGURE_MAXIMUM_USER_EXTENSIONS + 1 ];
  #endif
  Configuration_Scheduler_node Scheduler_nodes[ _CONFIGURE_SCHEDULER_COUNT ];
  RTEMS_API_Control API_RTEMS;
  #ifdef RTEMS_POSIX_API
    POSIX_API_Control API_POSIX;
  #endif
  #if CONFIGURE_MAXIMUM_THREAD_NAME_SIZE > 1
    char name[ CONFIGURE_MAXIMUM_THREAD_NAME_SIZE ];
  #endif
  #if defined(_CONFIGURE_ENABLE_NEWLIB_REENTRANCY) && \
    !defined(_REENT_THREAD_LOCAL)
    struct _reent Newlib;
  #endif
};

#define THREAD_INFORMATION_DEFINE( name, api, cls, max ) \
...
static ... \
Thread_Configured_control \
name##_Objects[ _Objects_Maximum_per_allocation( max ) ]; \
...


I don't see any static initialization of struct _Thread_Control::extensions
nor any member initialization of the name##_Objects, and even then that
is all legal in any arrangement:

truct flex      { int length; char data[]; };
struct mid_flex { int m; struct flex flex_data; int n; int o; };
struct end_flex { int m; int n; struct flex flex_data; };

struct flex f = { .length = 2 };
struct mid_flex m = { .m = 5 };
struct end_flex e = { .m = 5 };

struct flex fa[4] = { { .length = 2 } };
struct mid_flex ma[4] = { { .m = 5 } };
struct end_flex ea[4] = { { .m = 5 } };

These all work.


But yes, I see why you can't move Thread_Control trivially to the end. It
looks like you're depending on the implicit overlapping memory locations
between struct _Thread_Control and things that include it as the first
struct member, like struct Thread_Configured_control above:

cpukit/score/src/threaditerate.c:      the_thread = (Thread_Control *) information->local_table[ index ];

(In the Linux kernel we found this kind of open casting to be very
fragile and instead use a horrific wrapper called "container_of"[3] that
does the pointer math (possibly to an offset of 0 for a direct cast) to
find the member.)

Anyway, for avoiding the warning, you can just keep using the extension
and add -Wno-... if it ever ends up in -Wall, or you can redefine struct
_Thread_Control to avoid having the "extensions" member at all. This is
what we've done in several cases in Linux. For example if we had this
again, but made to look more like Thread_Control:

struct flex     { int foo; int bar; char data[]; };
struct mid_flex { struct flex hdr; int n; int o; };

It could be changed to:

struct flex_hdr { int foo; int bar; };
struct flex     { struct flex_hdr hdr; char data[]; };
struct mid_flex { struct flex_hdr hdr; int n; int o; };

This has some collateral changes needed to reference the struct flex_hdr
members from struct flex now (f->hdr.foo instead of f->foo). Sometimes
this can be avoided by using a union, as I did in a recent refactoring
in Linux: [4]

For more complex cases in Linux we've handled this by using our
"struct_group"[5] macro, which allows for a union and tagged struct to
be constructed:

struct flex {
	__struct_group(flex_hdr, hdr,,
		int foo;
		int bar;
	);
	char data[];
};
struct mid_flex { struct flex_hdr hdr; int n; int o; };

Then struct flex member names don't have to change, but if anything is
trying to get at struct flex::data through struct mid_flex::hdr, that'll
need casting. But it _shouldn't_ since it has "n" and "o".

-Kees

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620122.html
[2] https://github.com/RTEMS/rtems
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h#n10
[4] https://git.kernel.org/linus/896880ff30866f386ebed14ab81ce1ad3710cfc4
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stddef.h?h=v6.8#n11

-- 
Kees Cook