[PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Cristian Rodríguez]

Currently GCC 13  -fstrict-flex-arrays cannot be used safely with user
code because the library makes use of pre-c99 flexible arrays.

This series introduces a macro to annotate such uses and grandfathers-in
this legacy use.

This series only cover the user-visible *library* types. the linux kernel types
are subject to a different patch set and discussion since the kernel has
moved on to c11 leaving pre-c99 compatibilty behind.


Cristian Rodríguez (6):
  Introduce __decl_is_flex_array macro
  dlfcn : annotate dls_serpath with __decl_is_flex_array
  inet: annotate  tu_data and tu_stuff with __decl_is_flex_array
  inet: annotate ip6r0_addr with __decl_is_flex_array
  io: annotate fts_name with __decl_is_flex_array
  misc: annotate q_data with __decl_is_flex_array

 dlfcn/dlfcn.h      | 2 +-
 inet/arpa/tftp.h   | 6 ++++--
 inet/netinet/ip6.h | 2 +-
 io/fts.h           | 4 ++--
 misc/search.h      | 2 +-
 misc/sys/cdefs.h   | 6 ++++++
 6 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH 1/6] Introduce __decl_is_flex_array macro

[Cristian Rodríguez]

Currently it is not possible to safely build user code
with GCC-13 -fstrict-flex-arrays due to the library
use of legacy fake flexible arrays and zero-length array gcc extension.

This commit introduces a __decl_is_flex_array macro intended to
grandfather-in this uses without modifying the relevant definitions.

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 misc/sys/cdefs.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 90c21e2703..183cc5e2c9 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -720,4 +720,10 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 # define __attribute_returns_twice__ /* Ignore.  */
 #endif
 
+#if __glibc_has_attribute (__strict_flex_array__)
+#define __decl_is_flex_array __attribute__ ((__strict_flex_array__(0)))
+#else
+#define __decl_is_flex_array
+#endif
+
 #endif	 /* sys/cdefs.h */
-- 
2.41.0

[PATCH 2/6] dlfcn : annotate dls_serpath with __decl_is_flex_array

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 dlfcn/dlfcn.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index c5d192597d..f3a2d087f2 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -192,7 +192,7 @@ typedef struct
      not support structs with flexible array members in unions.  */
   __extension__ union
   {
-    Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
+    Dl_serpath dls_serpath[0] __decl_is_flex_array; /* Actually longer, dls_cnt elements.  */
     Dl_serpath __dls_serpath_pad[1];
   };
 # else
-- 
2.41.0

[PATCH 3/6] inet: annotate tu_data and tu_stuff with __decl_is_flex_array

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 inet/arpa/tftp.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/inet/arpa/tftp.h b/inet/arpa/tftp.h
index 86e0b6e814..43d1c30fe2 100644
--- a/inet/arpa/tftp.h
+++ b/inet/arpa/tftp.h
@@ -32,6 +32,8 @@
 #ifndef _ARPA_TFTP_H
 #define	_ARPA_TFTP_H 1
 
+#include <features.h>
+
 /*
  * Trivial File Transfer Protocol (IEN-133)
  */
@@ -55,9 +57,9 @@ struct	tftphdr {
 				unsigned short	tu_block;	/* block # */
 				short	tu_code;		/* error code */
 			} __attribute__ ((__packed__)) th_u3;
-			char tu_data[0];	/* data or error string */
+			char tu_data[0] __decl_is_flex_array;	/* data or error string */
 		} __attribute__ ((__packed__)) th_u2;
-		char	tu_stuff[0];		/* request packet stuff */
+		char	tu_stuff[0] __decl_is_flex_array;	/* request packet stuff */
 	} __attribute__ ((__packed__)) th_u1;
 } __attribute__ ((__packed__));
 
-- 
2.41.0

[PATCH 4/6] inet: annotate ip6r0_addr with __decl_is_flex_array

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 inet/netinet/ip6.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/inet/netinet/ip6.h b/inet/netinet/ip6.h
index feb66a15b9..24a96bac43 100644
--- a/inet/netinet/ip6.h
+++ b/inet/netinet/ip6.h
@@ -89,7 +89,7 @@ struct ip6_rthdr0
     uint8_t  ip6r0_reserved;	/* reserved field */
     uint8_t  ip6r0_slmap[3];	/* strict/loose bit map */
     /* followed by up to 127 struct in6_addr */
-    struct in6_addr ip6r0_addr[0];
+    struct in6_addr ip6r0_addr[0] __decl_is_flex_array;
   };
 
 /* Fragment header */
-- 
2.41.0

[PATCH 5/6] io: annotate fts_name with __decl_is_flex_array

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 io/fts.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/fts.h b/io/fts.h
index 493f494e06..dbafebaf1f 100644
--- a/io/fts.h
+++ b/io/fts.h
@@ -143,7 +143,7 @@ typedef struct _ftsent {
 	unsigned short fts_instr;	/* fts_set() instructions */
 
 	struct stat *fts_statp;		/* stat(2) information */
-	char fts_name[1];		/* file name */
+	char fts_name[1] __decl_is_flex_array;	/* file name */
 } FTSENT;
 
 #ifdef __USE_LARGEFILE64
@@ -173,7 +173,7 @@ typedef struct _ftsent64 {
 	unsigned short fts_instr;	/* fts_set() instructions */
 
 	struct stat64 *fts_statp;	/* stat(2) information */
-	char fts_name[1];		/* file name */
+	char fts_name[1] __decl_is_flex_array;	/* file name */
 } FTSENT64;
 #endif
 
-- 
2.41.0

[PATCH 6/6] misc: annotate q_data with __decl_is_flex_array

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <cristian@rodriguez.im>
---
 misc/search.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/search.h b/misc/search.h
index 30191d40cc..f1b1e63cae 100644
--- a/misc/search.h
+++ b/misc/search.h
@@ -35,7 +35,7 @@ struct qelem
   {
     struct qelem *q_forw;
     struct qelem *q_back;
-    char q_data[1];
+    char q_data[1] __decl_is_flex_array;
   };
 # endif
 
-- 
2.41.0

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Mon, Jul 10, 2023 at 11:05 AM Cristian Rodríguez <cristian@rodriguez.im>
wrote:

> Currently GCC 13  -fstrict-flex-arrays cannot be used safely with user
> code because the library makes use of pre-c99 flexible arrays.
>
> This series introduces a macro to annotate such uses and grandfathers-in
> this legacy use.
>
> This series only cover the user-visible *library* types. the linux kernel
> types
> are subject to a different patch set and discussion since the kernel has
> moved on to c11 leaving pre-c99 compatibilty behind.
>
>
>
>
Any comments about this series.. ? as is it should introduce no
regressions.

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Adhemerval Zanella Netto]

On 19/07/23 09:40, Cristian Rodríguez via Libc-alpha wrote:
> On Mon, Jul 10, 2023 at 11:05 AM Cristian Rodríguez <cristian@rodriguez.im>
> wrote:
> 
>> Currently GCC 13  -fstrict-flex-arrays cannot be used safely with user
>> code because the library makes use of pre-c99 flexible arrays.
>>
>> This series introduces a macro to annotate such uses and grandfathers-in
>> this legacy use.
>>
>> This series only cover the user-visible *library* types. the linux kernel
>> types
>> are subject to a different patch set and discussion since the kernel has
>> moved on to c11 leaving pre-c99 compatibilty behind.
>>
>>
>>
>>
> Any comments about this series.. ? as is it should introduce no
> regressions.


Can't we use the already define __flexarr/__glibc_c99_flexarr_available macro 
instead? Or do we really need to handle the usage of  -fstrict-flex-arrays
with pre-c99 usage?

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Wed, Jul 19, 2023 at 10:27 AM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

>
>
> On 19/07/23 09:40, Cristian Rodríguez via Libc-alpha wrote:
> > On Mon, Jul 10, 2023 at 11:05 AM Cristian Rodríguez <
> cristian@rodriguez.im>
> > wrote:
> >
> >> Currently GCC 13  -fstrict-flex-arrays cannot be used safely with user
> >> code because the library makes use of pre-c99 flexible arrays.
> >>
> >> This series introduces a macro to annotate such uses and grandfathers-in
> >> this legacy use.
> >>
> >> This series only cover the user-visible *library* types. the linux
> kernel
> >> types
> >> are subject to a different patch set and discussion since the kernel has
> >> moved on to c11 leaving pre-c99 compatibilty behind.
> >>
> >>
> >>
> >>
> > Any comments about this series.. ? as is it should introduce no
> > regressions.
>
>
> Can't we use the already define __flexarr/__glibc_c99_flexarr_available
> macro
> instead?


If I did only that, sizeof public structures may change causing an abi
break.
if breaking the ABi is an option then one can switch completely to c99
flexible arrays and using/importing DECL_FLEX_ARRAY macro from the linux
kernel and move on.

I concluded it was better to grandfather them in and leave them as special
cases is the less painful solution for user visible parts, internals can
just  use standard flex arrays.

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Adhemerval Zanella Netto]

On 19/07/23 11:57, Cristian Rodríguez wrote:
> 
> 
> On Wed, Jul 19, 2023 at 10:27 AM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
> 
> 
> 
>     On 19/07/23 09:40, Cristian Rodríguez via Libc-alpha wrote:
>     > On Mon, Jul 10, 2023 at 11:05 AM Cristian Rodríguez <cristian@rodriguez.im <mailto:cristian@rodriguez.im>>
>     > wrote:
>     >
>     >> Currently GCC 13  -fstrict-flex-arrays cannot be used safely with user
>     >> code because the library makes use of pre-c99 flexible arrays.
>     >>
>     >> This series introduces a macro to annotate such uses and grandfathers-in
>     >> this legacy use.
>     >>
>     >> This series only cover the user-visible *library* types. the linux kernel
>     >> types
>     >> are subject to a different patch set and discussion since the kernel has
>     >> moved on to c11 leaving pre-c99 compatibilty behind.
>     >>
>     >>
>     >>
>     >>
>     > Any comments about this series.. ? as is it should introduce no
>     > regressions.
> 
> 
>     Can't we use the already define __flexarr/__glibc_c99_flexarr_available macro
>     instead? 
> 
> 
> If I did only that, sizeof public structures may change causing an abi break. 
> if breaking the ABi is an option then one can switch completely to c99 flexible arrays and using/importing DECL_FLEX_ARRAY macro from the linux kernel and move on.
> 
> I concluded it was better to grandfather them in and leave them as special cases is the less painful solution for user visible parts, internals can just  use standard flex arrays.
>  

Fair enough, it is unfortunate that we had to use old flexible array syntax
on public headers.

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On Wed, Jul 19, 2023 at 1:48 PM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

>
> Fair enough, it is unfortunate that we had to use old flexible array syntax
> on public headers.
>

Any hope on getting this merged or get other feedback ?

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Paul Eggert]

On 2023-07-19 07:57, Cristian Rodríguez via Libc-alpha wrote:
>> Can't we use the already define __flexarr/__glibc_c99_flexarr_available
>> macro
>> instead?
> 
> If I did only that, sizeof public structures may change causing an abi
> break.

Why would it cause an ABI break?

For example, no program's machine code should depend on sizeof (FTSENT). 
fts.h's FTSENT is supposed to be an incomplete data type, like FILE: the 
system allocates FTSENT objects, not user code. So I don't see the harm 
in replacing [1] with [] in the definition of FTSENT's fts_name member, 
if the compiler supports flex arrays.

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2064 bytes --]

Hi Paul, Cristian,

On 2023-08-12 00:10, Paul Eggert wrote:
> On 2023-07-19 07:57, Cristian Rodríguez via Libc-alpha wrote:
>>> Can't we use the already define __flexarr/__glibc_c99_flexarr_available
>>> macro
>>> instead?
>>
>> If I did only that, sizeof public structures may change causing an abi
>> break.
> 
> Why would it cause an ABI break?
> 
> For example, no program's machine code should depend on sizeof (FTSENT).

+1


> fts.h's FTSENT is supposed to be an incomplete data type, like FILE: the 
> system allocates FTSENT objects, not user code. So I don't see the harm 
> in replacing [1] with [] in the definition of FTSENT's fts_name member, 
> if the compiler supports flex arrays.
> 

I'll raise the bid, and claim that no valid program can depend on
sizeof() of any struct with a flexible array member, of any kind (old
`[1]`, zero-length `[0]`, nor standard `[]`).

It is conceptually wrong to calculate sizeof() it, and the right thing to
do is to call offseof(s, fam).

In the best case, sizeof() will return the same as offsetof(3), and the
program will happen to work.  But in some cases, it will return a value
slightly bigger (ISO C guarantees that it will not be smaller, for no
reason I think), and in those cases, the program must have a bug.  That
bug can either be benign, and the program will just be wasting a few
bytes, or it can be bad, and read memory a few bytes off from where it
was written (still within the allocation bounds, so it won't crash; at
least not directly due to that, but maybe by a secondary bug).

See this GCC thread where I propose adding a warning when
sizeof(flexi_struct) is used in arithmetics.  There's an example program
showing why sizeof() is (almost) always a bad thing with these structures.
In the only use case where sizeof() is valid, reducing it wouldn't
change program behavior (it's within a malloc(MAX(sizeof(s), ...));).

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2322 bytes --]

On 2023-08-12 13:07, Alejandro Colomar wrote:
> Hi Paul, Cristian,
> 
> On 2023-08-12 00:10, Paul Eggert wrote:
>> On 2023-07-19 07:57, Cristian Rodríguez via Libc-alpha wrote:
>>>> Can't we use the already define __flexarr/__glibc_c99_flexarr_available
>>>> macro
>>>> instead?
>>>
>>> If I did only that, sizeof public structures may change causing an abi
>>> break.
>>
>> Why would it cause an ABI break?
>>
>> For example, no program's machine code should depend on sizeof (FTSENT).
> 
> +1
> 
> 
>> fts.h's FTSENT is supposed to be an incomplete data type, like FILE: the 
>> system allocates FTSENT objects, not user code. So I don't see the harm 
>> in replacing [1] with [] in the definition of FTSENT's fts_name member, 
>> if the compiler supports flex arrays.
>>
> 
> I'll raise the bid, and claim that no valid program can depend on
> sizeof() of any struct with a flexible array member, of any kind (old
> `[1]`, zero-length `[0]`, nor standard `[]`).
> 
> It is conceptually wrong to calculate sizeof() it, and the right thing to
> do is to call offseof(s, fam).
> 
> In the best case, sizeof() will return the same as offsetof(3), and the
> program will happen to work.  But in some cases, it will return a value
> slightly bigger (ISO C guarantees that it will not be smaller, for no
> reason I think), and in those cases, the program must have a bug.  That
> bug can either be benign, and the program will just be wasting a few
> bytes, or it can be bad, and read memory a few bytes off from where it
> was written (still within the allocation bounds, so it won't crash; at
> least not directly due to that, but maybe by a secondary bug).
> 
> See this GCC thread where I propose adding a warning when

Oops, I forgot to paste the link.

<https://inbox.sourceware.org/gcc/dac8afb7-5026-c702-85d2-c3ad977d9a48@kernel.org/T/>

> sizeof(flexi_struct) is used in arithmetics.  There's an example program
> showing why sizeof() is (almost) always a bad thing with these structures.
> In the only use case where sizeof() is valid, reducing it wouldn't
> change program behavior (it's within a malloc(MAX(sizeof(s), ...));).
> 
> Cheers,
> Alex
> 

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On Fri, Aug 11, 2023 at 6:10 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 2023-07-19 07:57, Cristian Rodríguez via Libc-alpha wrote:
> >> Can't we use the already define __flexarr/__glibc_c99_flexarr_available
> >> macro
> >> instead?
> >
> > If I did only that, sizeof public structures may change causing an abi
> > break.
>
> Why would it cause an ABI break?
>
> For example, no program's machine code should depend on sizeof (FTSENT).
> fts.h's FTSENT is supposed to be an incomplete data type, like FILE: the
> system allocates FTSENT objects, not user code. So I don't see the harm
> in replacing [1] with [] in the definition of FTSENT's fts_name member,
> if the compiler supports flex arrays.
>

I , unsurprisingly, agree with your rationale. been just extremely
conservative to avoid breaking even wrong programs..all this would be
easier if there was a way to "error: sizeof cannot be used correctly on
struct blabla"  and fail compile in the first place.

Re: [PATCH 0/6] Support use of -fstrict-flex-arrays in user code

[Paul Eggert]

On 2023-08-12 07:53, Cristian Rodríguez wrote:
> I , unsurprisingly, agree with your rationale.

Then let's do that instead.

> been just extremely
> conservative to avoid breaking even wrong programs.

I can't offhand see what plausible-but-wrong program would break.

One can imagine implausible scenarios where these invalid programs would 
break. But they're so unlikely we needn't worry about them.