| Message ID | 87a7bjkxhv.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | locale: Avoid zero-length array in _nl_category_names [BZ #24962] | expand |
On 9/5/19 1:34 AM, Florian Weimer wrote: > (Martin, can you give this a test using GCC 10? I don't expect any > issues because the zero-length array is completely gone. The patch > passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) > > The union wrapper is unnecessary because C allows to read any object > as a sequence of chars. > > 2019-09-05 Florian Weimer <fweimer@redhat.com> > > [BZ #24962] > * locale/localeinfo.h (_nl_category_names): Remove union wrapper. > (_nl_category_names_get): New function. > * intl/dcigettext.c (category_to_name): Call it. > * locale/findlocale.c (_nl_find_locale): Likewise. > * intl/loadlocale.c (_nl_load_locale): Likewise. > * locale/newlocale.c (__newlocale): Likewise. > * locale/setlocale.c (_nl_category_names): Adjust definition. > (_nl_category_name_idxs): Likewise. > (new_composite_name): Call _nl_category_names_get. > (setlocale): Likewise. Might be easier for me than Martin. I just threw it into my github repo, so my tester will pick it up as of now. Will report back in a few hours. jeff
On 9/5/19 7:25 AM, Jeff Law wrote: > On 9/5/19 1:34 AM, Florian Weimer wrote: >> (Martin, can you give this a test using GCC 10? I don't expect any >> issues because the zero-length array is completely gone. The patch >> passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) >> >> The union wrapper is unnecessary because C allows to read any object >> as a sequence of chars. >> >> 2019-09-05 Florian Weimer <fweimer@redhat.com> >> >> [BZ #24962] >> * locale/localeinfo.h (_nl_category_names): Remove union wrapper. >> (_nl_category_names_get): New function. >> * intl/dcigettext.c (category_to_name): Call it. >> * locale/findlocale.c (_nl_find_locale): Likewise. >> * intl/loadlocale.c (_nl_load_locale): Likewise. >> * locale/newlocale.c (__newlocale): Likewise. >> * locale/setlocale.c (_nl_category_names): Adjust definition. >> (_nl_category_name_idxs): Likewise. >> (new_composite_name): Call _nl_category_names_get. >> (setlocale): Likewise. > Might be easier for me than Martin. I just threw it into my github > repo, so my tester will pick it up as of now. Will report back in a few > hours. Thanks Jeff. I'm sure your coverage will be more comprehensive than mine. (The warning Florian is fixing only shows up with the patch for PR 91631 I posted yesterday so you need both in your tested to verify them.) It's pretty easy for me to test it on x86_64-linux: with your patch, Florian, and my enhanced GCC 10 I don't see any warnings. Martin
* Martin Sebor: > On 9/5/19 7:25 AM, Jeff Law wrote: >> On 9/5/19 1:34 AM, Florian Weimer wrote: >>> (Martin, can you give this a test using GCC 10? I don't expect any >>> issues because the zero-length array is completely gone. The patch >>> passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) >>> >>> The union wrapper is unnecessary because C allows to read any object >>> as a sequence of chars. >>> >>> 2019-09-05 Florian Weimer <fweimer@redhat.com> >>> >>> [BZ #24962] >>> * locale/localeinfo.h (_nl_category_names): Remove union wrapper. >>> (_nl_category_names_get): New function. >>> * intl/dcigettext.c (category_to_name): Call it. >>> * locale/findlocale.c (_nl_find_locale): Likewise. >>> * intl/loadlocale.c (_nl_load_locale): Likewise. >>> * locale/newlocale.c (__newlocale): Likewise. >>> * locale/setlocale.c (_nl_category_names): Adjust definition. >>> (_nl_category_name_idxs): Likewise. >>> (new_composite_name): Call _nl_category_names_get. >>> (setlocale): Likewise. >> Might be easier for me than Martin. I just threw it into my github >> repo, so my tester will pick it up as of now. Will report back in a few >> hours. > > Thanks Jeff. I'm sure your coverage will be more comprehensive > than mine. (The warning Florian is fixing only shows up with > the patch for PR 91631 I posted yesterday so you need both in > your tested to verify them.) > > It's pretty easy for me to test it on x86_64-linux: with your > patch, Florian, and my enhanced GCC 10 I don't see any warnings. Thanks. Do you (or anyone else) have any comments on the patch itself? I need some sort of review before I can push it. 8-) Florian
On 9/5/19 8:29 AM, Florian Weimer wrote: > * Martin Sebor: > >> On 9/5/19 7:25 AM, Jeff Law wrote: >>> On 9/5/19 1:34 AM, Florian Weimer wrote: >>>> (Martin, can you give this a test using GCC 10? I don't expect any >>>> issues because the zero-length array is completely gone. The patch >>>> passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) >>>> >>>> The union wrapper is unnecessary because C allows to read any object >>>> as a sequence of chars. >>>> >>>> 2019-09-05 Florian Weimer <fweimer@redhat.com> >>>> >>>> [BZ #24962] >>>> * locale/localeinfo.h (_nl_category_names): Remove union wrapper. >>>> (_nl_category_names_get): New function. >>>> * intl/dcigettext.c (category_to_name): Call it. >>>> * locale/findlocale.c (_nl_find_locale): Likewise. >>>> * intl/loadlocale.c (_nl_load_locale): Likewise. >>>> * locale/newlocale.c (__newlocale): Likewise. >>>> * locale/setlocale.c (_nl_category_names): Adjust definition. >>>> (_nl_category_name_idxs): Likewise. >>>> (new_composite_name): Call _nl_category_names_get. >>>> (setlocale): Likewise. >>> Might be easier for me than Martin. I just threw it into my github >>> repo, so my tester will pick it up as of now. Will report back in a few >>> hours. >> >> Thanks Jeff. I'm sure your coverage will be more comprehensive >> than mine. (The warning Florian is fixing only shows up with >> the patch for PR 91631 I posted yesterday so you need both in >> your tested to verify them.) >> >> It's pretty easy for me to test it on x86_64-linux: with your >> patch, Florian, and my enhanced GCC 10 I don't see any warnings. > > Thanks. Do you (or anyone else) have any comments on the patch itself? > I need some sort of review before I can push it. 8-) It's close to what I suggested in the bug so LGTM :) As an aside, an anonymous struct in a union was among the issues I mentioned in the other thread (see N2038), so it's good to get rid of even if this code is only compiled with GCC (and maybe also Clang?) Martin
On 9/5/19 8:26 AM, Martin Sebor wrote: > On 9/5/19 7:25 AM, Jeff Law wrote: >> On 9/5/19 1:34 AM, Florian Weimer wrote: >>> (Martin, can you give this a test using GCC 10? I don't expect any >>> issues because the zero-length array is completely gone. The patch >>> passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) >>> >>> The union wrapper is unnecessary because C allows to read any object >>> as a sequence of chars. >>> >>> 2019-09-05 Florian Weimer <fweimer@redhat.com> >>> >>> [BZ #24962] >>> * locale/localeinfo.h (_nl_category_names): Remove union wrapper. >>> (_nl_category_names_get): New function. >>> * intl/dcigettext.c (category_to_name): Call it. >>> * locale/findlocale.c (_nl_find_locale): Likewise. >>> * intl/loadlocale.c (_nl_load_locale): Likewise. >>> * locale/newlocale.c (__newlocale): Likewise. >>> * locale/setlocale.c (_nl_category_names): Adjust definition. >>> (_nl_category_name_idxs): Likewise. >>> (new_composite_name): Call _nl_category_names_get. >>> (setlocale): Likewise. >> Might be easier for me than Martin. I just threw it into my github >> repo, so my tester will pick it up as of now. Will report back in a few >> hours. > > Thanks Jeff. I'm sure your coverage will be more comprehensive > than mine. (The warning Florian is fixing only shows up with > the patch for PR 91631 I posted yesterday so you need both in > your tested to verify them.) > > It's pretty easy for me to test it on x86_64-linux: with your > patch, Florian, and my enhanced GCC 10 I don't see any warnings. ACK. WIll drop in yesterday's GCC patch and restart the relevant builds. jeff
On 9/5/19 3:34 AM, Florian Weimer wrote: > (Martin, can you give this a test using GCC 10? I don't expect any > issues because the zero-length array is completely gone. The patch > passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) > > The union wrapper is unnecessary because C allows to read any object > as a sequence of chars. OK for master. Changes all 8 expected places. Fix confirmed by Martin. Looks good to me. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > 2019-09-05 Florian Weimer <fweimer@redhat.com> > > [BZ #24962] > * locale/localeinfo.h (_nl_category_names): Remove union wrapper. > (_nl_category_names_get): New function. > * intl/dcigettext.c (category_to_name): Call it. > * locale/findlocale.c (_nl_find_locale): Likewise. > * intl/loadlocale.c (_nl_load_locale): Likewise. > * locale/newlocale.c (__newlocale): Likewise. > * locale/setlocale.c (_nl_category_names): Adjust definition. > (_nl_category_name_idxs): Likewise. > (new_composite_name): Call _nl_category_names_get. > (setlocale): Likewise. > > diff --git a/intl/dcigettext.c b/intl/dcigettext.c > index 4df93e2a8e..11f3749457 100644 > --- a/intl/dcigettext.c > +++ b/intl/dcigettext.c > @@ -360,8 +360,7 @@ static const char *guess_category_value (int category, > > #ifdef _LIBC > # include "../locale/localeinfo.h" > -# define category_to_name(category) \ > - _nl_category_names.str + _nl_category_name_idxs[category] > +# define category_to_name(category) _nl_category_names_get (category) OK. 1/8 > #else > static const char *category_to_name (int category); > #endif > diff --git a/locale/findlocale.c b/locale/findlocale.c > index 9af605bd64..28b0226265 100644 > --- a/locale/findlocale.c > +++ b/locale/findlocale.c > @@ -118,8 +118,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > variables. */ > cloc_name = getenv ("LC_ALL"); > if (!name_present (cloc_name)) > - cloc_name = getenv (_nl_category_names.str > - + _nl_category_name_idxs[category]); > + cloc_name = getenv (_nl_category_names_get (category)); OK. 2/8 > if (!name_present (cloc_name)) > cloc_name = getenv ("LANG"); > if (!name_present (cloc_name)) > @@ -207,8 +206,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > locale_path, locale_path_len, mask, > language, territory, codeset, > normalized_codeset, modifier, > - _nl_category_names.str > - + _nl_category_name_idxs[category], 0); > + _nl_category_names_get (category), 0); OK. 3/8 > > if (locale_file == NULL) > { > @@ -218,8 +216,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > locale_path, locale_path_len, mask, > language, territory, codeset, > normalized_codeset, modifier, > - _nl_category_names.str > - + _nl_category_name_idxs[category], 1); > + _nl_category_names_get (category), 1); OK. 4/8 > if (locale_file == NULL) > /* This means we are out of core. */ > return NULL; > diff --git a/locale/loadlocale.c b/locale/loadlocale.c > index 571c94e1de..ff578f6416 100644 > --- a/locale/loadlocale.c > +++ b/locale/loadlocale.c > @@ -199,8 +199,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category) > newp = (char *) alloca (filenamelen > + 5 + _nl_category_name_sizes[category] + 1); > __mempcpy (__mempcpy (__mempcpy (newp, file->filename, filenamelen), > - "/SYS_", 5), > - _nl_category_names.str + _nl_category_name_idxs[category], > + "/SYS_", 5), _nl_category_names_get (category), OK. 5/8 > _nl_category_name_sizes[category] + 1); > > fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC); > diff --git a/locale/localeinfo.h b/locale/localeinfo.h > index 7c1cc3eecb..0e2a0d7e49 100644 > --- a/locale/localeinfo.h > +++ b/locale/localeinfo.h > @@ -183,23 +183,29 @@ enum > #define _ISCTYPE(c, desc) \ > (((((const uint32_t *) (desc)) - 8)[(c) >> 5] >> ((c) & 0x1f)) & 1) > > -/* Category name handling variables. */ > +/* Category name handling variables. Concatenate all the strings in a > + single object to minimize relocations. Individual strings can be > + accessed using _nl_category_names. */ > #define CATNAMEMF(line) CATNAMEMF1 (line) > #define CATNAMEMF1(line) str##line > -extern const union catnamestr_t > +extern const struct catnamestr_t OK. Use struct. > { > - struct > - { > #define DEFINE_CATEGORY(category, category_name, items, a) \ > - char CATNAMEMF (__LINE__)[sizeof (category_name)]; > + char CATNAMEMF (__LINE__)[sizeof (category_name)]; > #include "categories.def" > #undef DEFINE_CATEGORY > - }; > - char str[0]; > } _nl_category_names attribute_hidden; > extern const uint8_t _nl_category_name_idxs[__LC_LAST] attribute_hidden; > extern const uint8_t _nl_category_name_sizes[__LC_LAST] attribute_hidden; > > +/* Return the name of the category INDEX, which must be nonnegative > + and less than _LC_LAST. */ > +static inline const char * > +_nl_category_names_get (int index) > +{ > + return (const char *) &_nl_category_names + _nl_category_name_idxs[index]; > +} OK. static inline with return that is cast. > + > /* Name of the standard locales. */ > extern const char _nl_C_name[] attribute_hidden; > extern const char _nl_POSIX_name[] attribute_hidden; > diff --git a/locale/newlocale.c b/locale/newlocale.c > index 8c5960a45d..561244245b 100644 > --- a/locale/newlocale.c > +++ b/locale/newlocale.c > @@ -131,8 +131,7 @@ __newlocale (int category_mask, const char *locale, locale_t base) > for (cnt = 0; cnt < __LC_LAST; ++cnt) > if (cnt != LC_ALL > && (size_t) (cp - np) == _nl_category_name_sizes[cnt] > - && memcmp (np, (_nl_category_names.str > - + _nl_category_name_idxs[cnt]), cp - np) == 0) > + && memcmp (np, (_nl_category_names_get (cnt)), cp - np) == 0) OK. 6/8 > break; > > if (cnt == __LC_LAST) > diff --git a/locale/setlocale.c b/locale/setlocale.c > index 9bd35454b9..264a1ecba7 100644 > --- a/locale/setlocale.c > +++ b/locale/setlocale.c > @@ -65,20 +65,18 @@ static char *const _nl_current_used[] = > > > /* Define an array of category names (also the environment variable names). */ > -const union catnamestr_t _nl_category_names attribute_hidden = > +const struct catnamestr_t _nl_category_names attribute_hidden = OK. Don't use union. > { > - { > #define DEFINE_CATEGORY(category, category_name, items, a) \ > - category_name, > + category_name, > #include "categories.def" > #undef DEFINE_CATEGORY > - } > }; > > const uint8_t _nl_category_name_idxs[__LC_LAST] attribute_hidden = > { > #define DEFINE_CATEGORY(category, category_name, items, a) \ > - [category] = offsetof (union catnamestr_t, CATNAMEMF (__LINE__)), > + [category] = offsetof (struct catnamestr_t, CATNAMEMF (__LINE__)), > #include "categories.def" > #undef DEFINE_CATEGORY > }; > @@ -180,7 +178,7 @@ new_composite_name (int category, const char *newnames[__LC_LAST]) > const char *name = (category == LC_ALL ? newnames[i] > : category == i ? newnames[0] > : _nl_global_locale.__names[i]); > - p = __stpcpy (p, _nl_category_names.str + _nl_category_name_idxs[i]); OK. 7/8 > + p = __stpcpy (p, _nl_category_names_get (i)); > *p++ = '='; > p = __stpcpy (p, name); > *p++ = ';'; > @@ -298,8 +296,7 @@ setlocale (int category, const char *locale) > for (cnt = 0; cnt < __LC_LAST; ++cnt) > if (cnt != LC_ALL > && (size_t) (cp - np) == _nl_category_name_sizes[cnt] > - && (memcmp (np, (_nl_category_names.str > - + _nl_category_name_idxs[cnt]), cp - np) > + && (memcmp (np, (_nl_category_names_get (cnt)), cp - np) OK. 8/8 > == 0)) > break; > >
On 9/5/19 1:34 AM, Florian Weimer wrote: > (Martin, can you give this a test using GCC 10? I don't expect any > issues because the zero-length array is completely gone. The patch > passed testing on x86_64-linux-gnu without regressions, but with GCC 9.) > > The union wrapper is unnecessary because C allows to read any object > as a sequence of chars. Seems to be working well. Nothing in glibc failing due to this. I'm seeing some GCC testsuite regressions, but I'm not convinced those are even related to Martin's work. x86_64, powerpc64le, aarch64, sh3, nios2, mips64el, mips64, s390, sh3eb, csky & microblaze are done. Others will follow as the day goes on, including things like m68k, alpha, various arm, i686, ppc64, ppc32, riscv64, etc. Jeff
diff --git a/intl/dcigettext.c b/intl/dcigettext.c index 4df93e2a8e..11f3749457 100644 --- a/intl/dcigettext.c +++ b/intl/dcigettext.c @@ -360,8 +360,7 @@ static const char *guess_category_value (int category, #ifdef _LIBC # include "../locale/localeinfo.h" -# define category_to_name(category) \ - _nl_category_names.str + _nl_category_name_idxs[category] +# define category_to_name(category) _nl_category_names_get (category) #else static const char *category_to_name (int category); #endif diff --git a/locale/findlocale.c b/locale/findlocale.c index 9af605bd64..28b0226265 100644 --- a/locale/findlocale.c +++ b/locale/findlocale.c @@ -118,8 +118,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, variables. */ cloc_name = getenv ("LC_ALL"); if (!name_present (cloc_name)) - cloc_name = getenv (_nl_category_names.str - + _nl_category_name_idxs[category]); + cloc_name = getenv (_nl_category_names_get (category)); if (!name_present (cloc_name)) cloc_name = getenv ("LANG"); if (!name_present (cloc_name)) @@ -207,8 +206,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, locale_path, locale_path_len, mask, language, territory, codeset, normalized_codeset, modifier, - _nl_category_names.str - + _nl_category_name_idxs[category], 0); + _nl_category_names_get (category), 0); if (locale_file == NULL) { @@ -218,8 +216,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, locale_path, locale_path_len, mask, language, territory, codeset, normalized_codeset, modifier, - _nl_category_names.str - + _nl_category_name_idxs[category], 1); + _nl_category_names_get (category), 1); if (locale_file == NULL) /* This means we are out of core. */ return NULL; diff --git a/locale/loadlocale.c b/locale/loadlocale.c index 571c94e1de..ff578f6416 100644 --- a/locale/loadlocale.c +++ b/locale/loadlocale.c @@ -199,8 +199,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category) newp = (char *) alloca (filenamelen + 5 + _nl_category_name_sizes[category] + 1); __mempcpy (__mempcpy (__mempcpy (newp, file->filename, filenamelen), - "/SYS_", 5), - _nl_category_names.str + _nl_category_name_idxs[category], + "/SYS_", 5), _nl_category_names_get (category), _nl_category_name_sizes[category] + 1); fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC); diff --git a/locale/localeinfo.h b/locale/localeinfo.h index 7c1cc3eecb..0e2a0d7e49 100644 --- a/locale/localeinfo.h +++ b/locale/localeinfo.h @@ -183,23 +183,29 @@ enum #define _ISCTYPE(c, desc) \ (((((const uint32_t *) (desc)) - 8)[(c) >> 5] >> ((c) & 0x1f)) & 1) -/* Category name handling variables. */ +/* Category name handling variables. Concatenate all the strings in a + single object to minimize relocations. Individual strings can be + accessed using _nl_category_names. */ #define CATNAMEMF(line) CATNAMEMF1 (line) #define CATNAMEMF1(line) str##line -extern const union catnamestr_t +extern const struct catnamestr_t { - struct - { #define DEFINE_CATEGORY(category, category_name, items, a) \ - char CATNAMEMF (__LINE__)[sizeof (category_name)]; + char CATNAMEMF (__LINE__)[sizeof (category_name)]; #include "categories.def" #undef DEFINE_CATEGORY - }; - char str[0]; } _nl_category_names attribute_hidden; extern const uint8_t _nl_category_name_idxs[__LC_LAST] attribute_hidden; extern const uint8_t _nl_category_name_sizes[__LC_LAST] attribute_hidden; +/* Return the name of the category INDEX, which must be nonnegative + and less than _LC_LAST. */ +static inline const char * +_nl_category_names_get (int index) +{ + return (const char *) &_nl_category_names + _nl_category_name_idxs[index]; +} + /* Name of the standard locales. */ extern const char _nl_C_name[] attribute_hidden; extern const char _nl_POSIX_name[] attribute_hidden; diff --git a/locale/newlocale.c b/locale/newlocale.c index 8c5960a45d..561244245b 100644 --- a/locale/newlocale.c +++ b/locale/newlocale.c @@ -131,8 +131,7 @@ __newlocale (int category_mask, const char *locale, locale_t base) for (cnt = 0; cnt < __LC_LAST; ++cnt) if (cnt != LC_ALL && (size_t) (cp - np) == _nl_category_name_sizes[cnt] - && memcmp (np, (_nl_category_names.str - + _nl_category_name_idxs[cnt]), cp - np) == 0) + && memcmp (np, (_nl_category_names_get (cnt)), cp - np) == 0) break; if (cnt == __LC_LAST) diff --git a/locale/setlocale.c b/locale/setlocale.c index 9bd35454b9..264a1ecba7 100644 --- a/locale/setlocale.c +++ b/locale/setlocale.c @@ -65,20 +65,18 @@ static char *const _nl_current_used[] = /* Define an array of category names (also the environment variable names). */ -const union catnamestr_t _nl_category_names attribute_hidden = +const struct catnamestr_t _nl_category_names attribute_hidden = { - { #define DEFINE_CATEGORY(category, category_name, items, a) \ - category_name, + category_name, #include "categories.def" #undef DEFINE_CATEGORY - } }; const uint8_t _nl_category_name_idxs[__LC_LAST] attribute_hidden = { #define DEFINE_CATEGORY(category, category_name, items, a) \ - [category] = offsetof (union catnamestr_t, CATNAMEMF (__LINE__)), + [category] = offsetof (struct catnamestr_t, CATNAMEMF (__LINE__)), #include "categories.def" #undef DEFINE_CATEGORY }; @@ -180,7 +178,7 @@ new_composite_name (int category, const char *newnames[__LC_LAST]) const char *name = (category == LC_ALL ? newnames[i] : category == i ? newnames[0] : _nl_global_locale.__names[i]); - p = __stpcpy (p, _nl_category_names.str + _nl_category_name_idxs[i]); + p = __stpcpy (p, _nl_category_names_get (i)); *p++ = '='; p = __stpcpy (p, name); *p++ = ';'; @@ -298,8 +296,7 @@ setlocale (int category, const char *locale) for (cnt = 0; cnt < __LC_LAST; ++cnt) if (cnt != LC_ALL && (size_t) (cp - np) == _nl_category_name_sizes[cnt] - && (memcmp (np, (_nl_category_names.str - + _nl_category_name_idxs[cnt]), cp - np) + && (memcmp (np, (_nl_category_names_get (cnt)), cp - np) == 0)) break;