Re: [PATCH] locale: Avoid zero-length array in _nl_category_names [BZ #24962]

[Carlos O'Donell <carlos at redhat dot com>]

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;
>  
> 


-- 
Cheers,
Carlos.