[PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

The dls_serpath path field, as an array of length 1, introduces
unexpected array subscript checks with some compilers.  Using a
zero-length array (a GNU extension) avoids that.  The anonymous union
preserves the original size of the type.

2019-05-23  Florian Weimer  <fweimer@redhat.com>

	[BZ #24166]
	* dlfcn/dlfcn.h (Dl_serinfo): Do not use array of length 1 for
	dls_serpath field.

diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index 896ad6fc9b..2ffb13d424 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -180,7 +180,17 @@ typedef struct
 {
   size_t dls_size;		/* Size in bytes of the whole buffer.  */
   unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
+# ifdef __GNUC__
+  /* This avoids an unwanted array subscript check by the compiler,
+     while preserving the size of the type.  */
+  __extension__ union
+  {
+    Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
+    Dl_serpath __dls_serpath_pad[1];
+  };
+# else /* !__GNUC__ */
   Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
+# endif /* !__GNUC__ */
 } Dl_serinfo;
 #endif /* __USE_GNU */
 

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Paul Eggert]

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

On 5/23/19 2:34 AM, Florian Weimer wrote:
> +# ifdef __GNUC__
> +  /* This avoids an unwanted array subscript check by the compiler,
> +     while preserving the size of the type.  */
> +  __extension__ union
> +  {
> +    Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
> +    Dl_serpath __dls_serpath_pad[1];
> +  };
> +# else /* !__GNUC__ */
>     Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
> +# endif /* !__GNUC__ */

Since this is actually a flexible array member, shouldn't we be using 
C99's support for that if available, instead? Something like the 
attached untested patch, say. We've been using a FLEXIBLE_ARRAY_MEMBER 
macro in Gnulib for quite some time to do this sort of thing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dlfcn.diff --]
[-- Type: text/x-patch; name="dlfcn.diff", Size: 1503 bytes --]

diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index 896ad6fc9b..01e6fdadc3 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -180,7 +180,7 @@ typedef struct
 {
   size_t dls_size;		/* Size in bytes of the whole buffer.  */
   unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
-  Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
+  Dl_serpath dls_serpath[__GLIBC_FLEXIBLE_ARRAY_MEMBER /* dls_cnt */];
 } Dl_serinfo;
 #endif /* __USE_GNU */
 
diff --git a/include/features.h b/include/features.h
index e016b3e5c7..9942984e57 100644
--- a/include/features.h
+++ b/include/features.h
@@ -141,6 +141,7 @@
 #undef	__KERNEL_STRICT_NAMES
 #undef	__GLIBC_USE_DEPRECATED_GETS
 #undef	__GLIBC_USE_DEPRECATED_SCANF
+#undef	__GLIBC_FLEXIBLE_ARRAY_MEMBER
 
 /* Suppress kernel-name space pollution unless user expressedly asks
    for it.  */
@@ -423,6 +424,15 @@
 # define __GLIBC_USE_DEPRECATED_SCANF 0
 #endif
 
+/* 'struct { ...; t m[__GLIBC_FLEXIBLE_ARRAY_MEMBER]; }’ declares a
+   structure with a flexible array member m at the end in C99 or later,
+   and a structure with a size-1 array member with earlier compilers.  */
+#if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
+# define __GLIBC_FLEXIBLE_ARRAY_MEMBER
+#else
+# define __GLIBC_FLEXIBLE_ARRAY_MEMBER 1
+#endif
+
 /* Get definitions of __STDC_* predefined macros, if the compiler has
    not preincluded this header automatically.  */
 #include <stdc-predef.h>

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Joseph Myers]

On Thu, 23 May 2019, Paul Eggert wrote:

> On 5/23/19 2:34 AM, Florian Weimer wrote:
> > +# ifdef __GNUC__
> > +  /* This avoids an unwanted array subscript check by the compiler,
> > +     while preserving the size of the type.  */
> > +  __extension__ union
> > +  {
> > +    Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
> > +    Dl_serpath __dls_serpath_pad[1];
> > +  };
> > +# else /* !__GNUC__ */
> >     Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.
> > */
> > +# endif /* !__GNUC__ */
> 
> Since this is actually a flexible array member, shouldn't we be using C99's
> support for that if available, instead? Something like the attached untested
> patch, say. We've been using a FLEXIBLE_ARRAY_MEMBER macro in Gnulib for quite
> some time to do this sort of thing.

Since we already have the __flexarr macro in sys/cdefs.h, I don't think 
having a slightly different __GLIBC_FLEXIBLE_ARRAY_MEMBER as well seems 
like a good idea.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

* Paul Eggert:

> On 5/23/19 2:34 AM, Florian Weimer wrote:
>> +# ifdef __GNUC__
>> +  /* This avoids an unwanted array subscript check by the compiler,
>> +     while preserving the size of the type.  */
>> +  __extension__ union
>> +  {
>> +    Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
>> +    Dl_serpath __dls_serpath_pad[1];
>> +  };
>> +# else /* !__GNUC__ */
>>     Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
>> +# endif /* !__GNUC__ */
>
> Since this is actually a flexible array member, shouldn't we be using
> C99's support for that if available, instead? Something like the
> attached untested patch, say. We've been using a FLEXIBLE_ARRAY_MEMBER
> macro in Gnulib for quite some time to do this sort of thing.

This changes the size of the type and is not source-code-compatible.  I
have not investigated whether the change is still reasonably safe, but
usually, wo do not make such changes.

Thanks,
Florian

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Paul Eggert]

On 5/24/19 1:42 AM, Florian Weimer wrote:
> This changes the size of the type and is not source-code-compatible.  I
> have not investigated whether the change is still reasonably safe, but
> usually, wo do not make such changes.

OK, in that case I suggest adding a comment explaining the situation, 
since it is a bit of a sore thumb. Something like the following perhaps? 
Or if this problem is likely to occur elsewhere, we could package the 
situation up into a macro and just use the macro here.

     /* An array of dls_cnt elements, each of type Dl_serpath.  */
   #if 0
     /* With no backward-compatibility concerns we’d use the following
        C99 flexible array member.  However, as this data structure
        predates C99 it had to contain a one-element array here, and we
        don't want to change the struct's size now.  */
     Dl_serpath dls_serpath[];
   #elif defined __GNUC__
     /* Avoid an unwanted array subscript check by the compiler, while
        preserving the size of the type.  */
     __extension__ union
     {
       Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
       Dl_serpath __dls_serpath_pad[1];
     };
   #else
     Dl_serpath dls_serpath[1];
   #endif

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

* Paul Eggert:

> On 5/24/19 1:42 AM, Florian Weimer wrote:
>> This changes the size of the type and is not source-code-compatible.  I
>> have not investigated whether the change is still reasonably safe, but
>> usually, wo do not make such changes.
>
> OK, in that case I suggest adding a comment explaining the situation,
> since it is a bit of a sore thumb. Something like the following
> perhaps? Or if this problem is likely to occur elsewhere, we could
> package the situation up into a macro and just use the macro here.
>
>     /* An array of dls_cnt elements, each of type Dl_serpath.  */
>   #if 0
>     /* With no backward-compatibility concerns we’d use the following
>        C99 flexible array member.  However, as this data structure
>        predates C99 it had to contain a one-element array here, and we
>        don't want to change the struct's size now.  */
>     Dl_serpath dls_serpath[];
>   #elif defined __GNUC__
>     /* Avoid an unwanted array subscript check by the compiler, while
>        preserving the size of the type.  */
>     __extension__ union
>     {
>       Dl_serpath dls_serpath[0]; /* Actually longer, dls_cnt elements.  */
>       Dl_serpath __dls_serpath_pad[1];
>     };
>   #else
>     Dl_serpath dls_serpath[1];
>   #endif

The real issue here is that GNU C allows nested flexible array members,
but not in unions.

I still think this is best discussed in the commit message because such
header comments tend not to age well, but I've proposed a patch below.

Thanks,
Florian

dlfcn: Avoid one-element flexible array in Dl_serinfo

The dls_serpath path field, as an array of length 1, introduces
unexpected array subscript checks with some compilers.

2019-05-27  Florian Weimer  <fweimer@redhat.com>

	[BZ #24166]
	* dlfcn/dlfcn.h (Dl_serinfo): Do not use array of length 1 for
	dls_serpath field.

diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index 896ad6fc9b..7107b3ea9a 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -180,7 +180,19 @@ typedef struct
 {
   size_t dls_size;		/* Size in bytes of the whole buffer.  */
   unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
+# ifdef __GNUC__
+  /* The zero-length array avoids an unwanted array subscript check by
+     the compiler, while the surrounding anonymous union preserves the
+     historic size of the type.  At the time of writing, GNU C does
+     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_pad[1];
+  };
+# else /* !__GNUC__ */
   Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
+# endif /* !__GNUC__ */
 } Dl_serinfo;
 #endif /* __USE_GNU */
 

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Paul Eggert]

Thanks, it looks OK to me.

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

* Florian Weimer:

> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
> index 896ad6fc9b..7107b3ea9a 100644
> --- a/dlfcn/dlfcn.h
> +++ b/dlfcn/dlfcn.h
> @@ -180,7 +180,19 @@ typedef struct
>  {
>    size_t dls_size;		/* Size in bytes of the whole buffer.  */
>    unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
> +# ifdef __GNUC__
> +  /* The zero-length array avoids an unwanted array subscript check by
> +     the compiler, while the surrounding anonymous union preserves the
> +     historic size of the type.  At the time of writing, GNU C does
> +     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_pad[1];
> +  };
> +# else /* !__GNUC__ */
>    Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
> +# endif /* !__GNUC__ */
>  } Dl_serinfo;
>  #endif /* __USE_GNU */
>  

It turns out that GCC 2.7.2.3 treats this anonymous union as a type
declaration and ignores it.  I will try to come up with the appropriate
__GNUC_PREREQ conditional.

(Don't get the wrong idea; we do not test regularly against older GCC
versions.)

Thanks,
Florian

[PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

The dls_serpath path field, as an array of length 1, introduces
unexpected array subscript checks with some compilers.

GCC versions before 3.0 treat the nested anonymous union as a
declaration of an unnamed type, and not as a member declaration,
so this construct cannot be used for these compilers.

2019-06-03  Florian Weimer  <fweimer@redhat.com>

	[BZ #24166]
	* dlfcn/dlfcn.h (Dl_serinfo): Do not use array of length 1 for
	dls_serpath field.

diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index 896ad6fc9b..c550371999 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -180,7 +180,19 @@ typedef struct
 {
   size_t dls_size;		/* Size in bytes of the whole buffer.  */
   unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
+# if __GNUC_PREREQ (3, 0)
+  /* The zero-length array avoids an unwanted array subscript check by
+     the compiler, while the surrounding anonymous union preserves the
+     historic size of the type.  At the time of writing, GNU C does
+     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_pad[1];
+  };
+# else
   Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
+# endif
 } Dl_serinfo;
 #endif /* __USE_GNU */
 

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Florian Weimer]

* Florian Weimer:

> The dls_serpath path field, as an array of length 1, introduces
> unexpected array subscript checks with some compilers.
>
> GCC versions before 3.0 treat the nested anonymous union as a
> declaration of an unnamed type, and not as a member declaration,
> so this construct cannot be used for these compilers.
>
> 2019-06-03  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #24166]
> 	* dlfcn/dlfcn.h (Dl_serinfo): Do not use array of length 1 for
> 	dls_serpath field.
>
> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
> index 896ad6fc9b..c550371999 100644
> --- a/dlfcn/dlfcn.h
> +++ b/dlfcn/dlfcn.h
> @@ -180,7 +180,19 @@ typedef struct
>  {
>    size_t dls_size;		/* Size in bytes of the whole buffer.  */
>    unsigned int dls_cnt;		/* Number of elements in `dls_serpath'.  */
> +# if __GNUC_PREREQ (3, 0)
> +  /* The zero-length array avoids an unwanted array subscript check by
> +     the compiler, while the surrounding anonymous union preserves the
> +     historic size of the type.  At the time of writing, GNU C does
> +     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_pad[1];
> +  };
> +# else
>    Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
> +# endif
>  } Dl_serinfo;
>  #endif /* __USE_GNU */
>  

Ping?

Thanks,
Florian

Re: [PATCH] dlfcn: Avoid one-element flexible array in Dl_serinfo

[Paul Eggert]

That looks OK to me, thanks. (Sorry, I thought I had already said "LGTM".)