Re: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

Re: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Kees Cook]

On Fri, Sep 08, 2023 at 06:14:38PM +0300, Alexey Dobriyan wrote:
> __DECLARE_FLEX_ARRAY(T, member) macro expands to
> 
> 	struct {
> 		struct {} __empty_member;
> 		T member[];
> 	};
> 
> which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,

Ewwww. Isn't this a bug in C++?

> changing UAPI structures layouts.
> 
> This can be fixed by expanding to
> 
> 	T member[];
> 
> Now g++ doesn't like "T member[]" either throwing errors on code like
> this:
> 
> 	struct S {
> 		union {
> 			T1 member1[];
> 			T2 member2[];
> 		};
> 	};
> 
> or
> 
> 	struct S {
> 		T member[];
> 	};
> So use
> 
> 	T member[0];
> 
> which seems to work and does the right thing wrt structure layout.

It seems sad to leave C++ broken, but I guess we have to do this.

Acked-by: Kees Cook <keescook@chromium.org>

> Fix header guard while I'm at it.

Hm, when did that get broken? Maybe that should be fixed separately?

> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Probably a Fixes: tag would be nice too.

-Kees

> ---
> 
>  include/uapi/linux/stddef.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/include/uapi/linux/stddef.h
> +++ b/include/uapi/linux/stddef.h
> @@ -39,6 +39,10 @@
>   * struct, it needs to be wrapped in an anonymous struct with at least 1
>   * named member, but that member can be empty.
>   */
> +#ifdef __cplusplus
> +#define __DECLARE_FLEX_ARRAY(T, member)		\
> +	T member[0]
> +#else
>  #define __DECLARE_FLEX_ARRAY(TYPE, NAME)	\
>  	struct { \
>  		struct { } __empty_ ## NAME; \
> @@ -49,3 +53,5 @@
>  #ifndef __counted_by
>  #define __counted_by(m)
>  #endif
> +
> +#endif

-- 
Kees Cook

Re: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Alexey Dobriyan]

On Fri, Sep 08, 2023 at 08:53:12AM -0700, Kees Cook wrote:
> On Fri, Sep 08, 2023 at 06:14:38PM +0300, Alexey Dobriyan wrote:
> > __DECLARE_FLEX_ARRAY(T, member) macro expands to
> > 
> > 	struct {
> > 		struct {} __empty_member;
> > 		T member[];
> > 	};
> > 
> > which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,
> 
> Ewwww. Isn't this a bug in C++?

Sort of, but it can't be fixed.

> > changing UAPI structures layouts.
> > 
> > This can be fixed by expanding to
> > 
> > 	T member[];
> > 
> > Now g++ doesn't like "T member[]" either throwing errors on code like
> > this:
> > 
> > 	struct S {
> > 		union {
> > 			T1 member1[];
> > 			T2 member2[];
> > 		};
> > 	};
> > 
> > or
> > 
> > 	struct S {
> > 		T member[];
> > 	};
> > So use
> > 
> > 	T member[0];
> > 
> > which seems to work and does the right thing wrt structure layout.
> 
> It seems sad to leave C++ broken, but I guess we have to do this.
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> > Fix header guard while I'm at it.
> 
> Hm, when did that get broken? Maybe that should be fixed separately?

By your last commit?

> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> 
> Probably a Fixes: tag would be nice too.

OK

Fixes: c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and identifier expansion")
Fixes: 3080ea5553cc ("stddef: Introduce DECLARE_FLEX_ARRAY() helper")

> >  include/uapi/linux/stddef.h |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > --- a/include/uapi/linux/stddef.h
> > +++ b/include/uapi/linux/stddef.h
> > @@ -39,6 +39,10 @@
> >   * struct, it needs to be wrapped in an anonymous struct with at least 1
> >   * named member, but that member can be empty.
> >   */
> > +#ifdef __cplusplus
> > +#define __DECLARE_FLEX_ARRAY(T, member)		\
> > +	T member[0]
> > +#else
> >  #define __DECLARE_FLEX_ARRAY(TYPE, NAME)	\
> >  	struct { \
> >  		struct { } __empty_ ## NAME; \
> > @@ -49,3 +53,5 @@
> >  #ifndef __counted_by
> >  #define __counted_by(m)
> >  #endif
> > +
> > +#endif
> 
> -- 
> Kees Cook

Re: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Kees Cook]

On Fri, Sep 08, 2023 at 07:03:17PM +0300, Alexey Dobriyan wrote:
> On Fri, Sep 08, 2023 at 08:53:12AM -0700, Kees Cook wrote:
> > On Fri, Sep 08, 2023 at 06:14:38PM +0300, Alexey Dobriyan wrote:
> > > __DECLARE_FLEX_ARRAY(T, member) macro expands to
> > > 
> > > 	struct {
> > > 		struct {} __empty_member;
> > > 		T member[];
> > > 	};
> > > 
> > > which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,
> > 
> > Ewwww. Isn't this a bug in C++?
> 
> Sort of, but it can't be fixed.
> 
> > > changing UAPI structures layouts.
> > > 
> > > This can be fixed by expanding to
> > > 
> > > 	T member[];
> > > 
> > > Now g++ doesn't like "T member[]" either throwing errors on code like
> > > this:
> > > 
> > > 	struct S {
> > > 		union {
> > > 			T1 member1[];
> > > 			T2 member2[];
> > > 		};
> > > 	};
> > > 
> > > or
> > > 
> > > 	struct S {
> > > 		T member[];
> > > 	};
> > > So use
> > > 
> > > 	T member[0];
> > > 
> > > which seems to work and does the right thing wrt structure layout.
> > 
> > It seems sad to leave C++ broken, but I guess we have to do this.
> > 
> > Acked-by: Kees Cook <keescook@chromium.org>
> > 
> > > Fix header guard while I'm at it.
> > 
> > Hm, when did that get broken? Maybe that should be fixed separately?
> 
> By your last commit?

:( Oops. I'm shocked this hasn't caused bigger problems.

> 
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > 
> > Probably a Fixes: tag would be nice too.
> 
> OK
> 
> Fixes: c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and identifier expansion")
> Fixes: 3080ea5553cc ("stddef: Introduce DECLARE_FLEX_ARRAY() helper")

Okay, can you please split the patch so they can be backported
separately? Then I'll get them landed, etc.

Thanks for fixing these!

-Kees

-- 
Kees Cook

RE: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[David Laight]

...
> Okay, can you please split the patch so they can be backported
> separately? Then I'll get them landed, etc.

Since the header with just the extra #endif is badly broken on C++
isn't it best to ensure they get back-ported together?
So one patch is probably better.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Alexey Dobriyan]

On Mon, Sep 11, 2023 at 08:19:20AM +0000, David Laight wrote:
> ...
> > Okay, can you please split the patch so they can be backported
> > separately? Then I'll get them landed, etc.
> 
> Since the header with just the extra #endif is badly broken on C++
> isn't it best to ensure they get back-ported together?
> So one patch is probably better.

Header guard misplacement is not a bug, file ends with:

	#ifndef __counted_by
	#define __counted_by(m)
	#endif

it is just looks confusing in the diff.

[PATCH v3 1/2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Alexey Dobriyan]

__DECLARE_FLEX_ARRAY(T, member) macro expands to

	struct {
		struct {} __empty_member;
		T member[];
	};

which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,
changing UAPI structures layouts.

This can be fixed by expanding to

	T member[];

Now g++ doesn't like "T member[]" either, throwing errors on
the following code:

	struct S {
		union {
			T1 member1[];
			T2 member2[];
		};
	};

or

	struct S {
		T member[];
	};

Use "T member[0];" which seems to work and does the right thing wrt
structure layout.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Fixes: 3080ea5553cc ("stddef: Introduce DECLARE_FLEX_ARRAY() helper")
---

 include/uapi/linux/stddef.h |    6 ++++++
 1 file changed, 6 insertions(+)

--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -29,6 +29,11 @@
 		struct TAG { MEMBERS } ATTRS NAME; \
 	}
 
+#ifdef __cplusplus
+/* sizeof(struct{}) is 1 in C++, not 0, can't use C version of the macro. */
+#define __DECLARE_FLEX_ARRAY(T, member)	\
+	T member[0]
+#else
 /**
  * __DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
  *
@@ -45,6 +50,7 @@
 		TYPE NAME[]; \
 	}
 #endif
+#endif
 
 #ifndef __counted_by
 #define __counted_by(m)

[PATCH v3 2/2] uapi: fix header guard in include/uapi/linux/stddef.h

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/uapi/linux/stddef.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -50,8 +50,9 @@
 		TYPE NAME[]; \
 	}
 #endif
-#endif
 
 #ifndef __counted_by
 #define __counted_by(m)
 #endif
+
+#endif

Re: [PATCH v3 1/2] uapi: fix __DECLARE_FLEX_ARRAY for C++

[Kees Cook]

On Tue, Sep 12, 2023 at 07:22:24PM +0300, Alexey Dobriyan wrote:
> __DECLARE_FLEX_ARRAY(T, member) macro expands to
> 
> 	struct {
> 		struct {} __empty_member;
> 		T member[];
> 	};
> 
> which is subtly wrong in C++ because sizeof(struct{}) is 1 not 0,
> changing UAPI structures layouts.

Looking at this again just now, what about using a 0-length array
instead of an anonymous struct?

https://godbolt.org/z/rGaxPWjef

Then we don't need an #ifdef at all...

 	struct {
 		int __empty_member[0];
 		T member[];
 	};

-Kees

-- 
Kees Cook