| Message ID | 87k1ehzf7o.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | dlfcn: Avoid one-element flexible array in Dl_serinfo | expand |
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.
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.
* 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
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
* 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 */
Thanks, it looks OK to me.
* 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
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 */