[BZ,19084] Make sure _nl_value_type_LC_<category> arrays have correct size.

  Otherwise if the last element of a category is an array (stringarray,
wstringarray etc.) this array will count only the first element
of the array element and will not load a category, will complain
that a locale file is too long.

Probably related with bug 19084.
---
 ChangeLog           | 4 ++++
 locale/loadlocale.c | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)
  

On 21 Mar 2016 10:10, Rafal Luzynski wrote:
> Otherwise if the last element of a category is an array (stringarray,
> wstringarray etc.) this array will count only the first element
> of the array element and will not load a category, will complain
> that a locale file is too long.

sorry, but i'm not seeing the problem here.  the type field is merely
an enum.  whether the last element is a stringarray (value of 2) or a
byte (value of 3), the array will still build up the same # of elements.

the _nl_value_type_LC_xxx array is indexing the _NL_xxx field only, not
the contents of it.

> --- a/locale/loadlocale.c
> +++ b/locale/loadlocale.c
> @@ -45,7 +45,8 @@ static const size_t _nl_category_num_items[] =
>  #define NO_PAREN(arg, rest...) arg, ##rest
>  
>  #define DEFINE_CATEGORY(category, category_name, items, a) \
> -static const enum value_type _nl_value_type_##category[] = { NO_PAREN items };
> +static const enum value_type _nl_value_type_##category     \
> +  [_NL_ITEM_INDEX (_NL_NUM_##category)] = { NO_PAREN items };
>  #define DEFINE_ELEMENT(element, element_name, optstd, type, rest...) \
>    [_NL_ITEM_INDEX (element)] = type,
>  #include "categories.def"

the code today will do for LC_IDENTIFICATION:
static const enum value_type _nl_value_type_LC_IDENTIFICATION[] = {
	[((int) (_NL_IDENTIFICATION_TITLE) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_SOURCE) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_ADDRESS) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_CONTACT) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_EMAIL) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_TEL) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_FAX) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_LANGUAGE) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_TERRITORY) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_AUDIENCE) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_APPLICATION) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_ABBREVIATION) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_REVISION) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_DATE) & 0xffff)] = string,
	[((int) (_NL_IDENTIFICATION_CATEGORY) & 0xffff)] = stringarray,
	[((int) (_NL_IDENTIFICATION_CODESET) & 0xffff)] = string,
};

these NL values are defined in langinfo.h and expand as:
  _NL_IDENTIFICATION_TITLE = (((12) << 16) | (0)),
  _NL_IDENTIFICATION_SOURCE,
  _NL_IDENTIFICATION_ADDRESS,
  _NL_IDENTIFICATION_CONTACT,
  _NL_IDENTIFICATION_EMAIL,
  _NL_IDENTIFICATION_TEL,
  _NL_IDENTIFICATION_FAX,
  _NL_IDENTIFICATION_LANGUAGE,
  _NL_IDENTIFICATION_TERRITORY,
  _NL_IDENTIFICATION_AUDIENCE,
  _NL_IDENTIFICATION_APPLICATION,
  _NL_IDENTIFICATION_ABBREVIATION,
  _NL_IDENTIFICATION_REVISION,
  _NL_IDENTIFICATION_DATE,
  _NL_IDENTIFICATION_CATEGORY,
  _NL_IDENTIFICATION_CODESET,
  _NL_NUM_LC_IDENTIFICATION,

_NL_IDENTIFICATION_CODESET is merely one larger than the
_NL_IDENTIFICATION_CATEGORY value.  if CODESET didn't exist,
then CATEGORY (a stringarray) would be at the end, but the
array would still be defined/calculated correctly.
-mike
  

Thank you for your reply, Mike. Please sese my answers below:

> On 21 Mar 2016 23:59 Mike Frysinger <vapier@gentoo.org> wrote:
>
> [...]
> sorry, but i'm not seeing the problem here. the type field is merely
> an enum. whether the last element is a stringarray (value of 2) or a
> byte (value of 3), the array will still build up the same # of elements.
> [...]
>
> the code today will do for LC_IDENTIFICATION:
> static const enum value_type _nl_value_type_LC_IDENTIFICATION[] = {
> [((int) (_NL_IDENTIFICATION_TITLE) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_SOURCE) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_ADDRESS) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_CONTACT) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_EMAIL) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_TEL) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_FAX) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_LANGUAGE) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_TERRITORY) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_AUDIENCE) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_APPLICATION) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_ABBREVIATION) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_REVISION) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_DATE) & 0xffff)] = string,
> [((int) (_NL_IDENTIFICATION_CATEGORY) & 0xffff)] = stringarray,
> [((int) (_NL_IDENTIFICATION_CODESET) & 0xffff)] = string,
> };
>
> these NL values are defined in langinfo.h and expand as:
> _NL_IDENTIFICATION_TITLE = (((12) << 16) | (0)),
> _NL_IDENTIFICATION_SOURCE,
> _NL_IDENTIFICATION_ADDRESS,
> _NL_IDENTIFICATION_CONTACT,
> _NL_IDENTIFICATION_EMAIL,
> _NL_IDENTIFICATION_TEL,
> _NL_IDENTIFICATION_FAX,
> _NL_IDENTIFICATION_LANGUAGE,
> _NL_IDENTIFICATION_TERRITORY,
> _NL_IDENTIFICATION_AUDIENCE,
> _NL_IDENTIFICATION_APPLICATION,
> _NL_IDENTIFICATION_ABBREVIATION,
> _NL_IDENTIFICATION_REVISION,
> _NL_IDENTIFICATION_DATE,
> _NL_IDENTIFICATION_CATEGORY,
> _NL_IDENTIFICATION_CODESET,
> _NL_NUM_LC_IDENTIFICATION,
>
> _NL_IDENTIFICATION_CODESET is merely one larger than the
> _NL_IDENTIFICATION_CATEGORY value. if CODESET didn't exist,
> then CATEGORY (a stringarray) would be at the end, but the
> array would still be defined/calculated correctly.
> -mike

Indeed, there is no bug for LC_IDENTIFICATION array because
_NL_IDENTIFICATION_CODESET is one larger than _NL_IDENTIFICATION_CATEGORY
although I wonder how to retrieve a specified element of
_NL_IDENTIFICATION_CATEGORY if there are no other indexes between
_NL_IDENTIFICATION_CATEGORY and _NL_IDENTIFICATION_CODESET.

I spotted this problem when working on bug 10871 [1]. I tried to add
ALTMON (and _NL_WALTMON) at the end of LC_TIME. The code expands to:

static const enum value_type _nl_value_type_LC_TIME [] = {
 [((int) (ABDAY_1) & 0xffff)] = stringarray,
 [((int) (DAY_1) & 0xffff)] = stringarray,
 [((int) (ABMON_1) & 0xffff)] = stringarray,
 [((int) (MON_1) & 0xffff)] = stringarray,
 [((int) (AM_STR) & 0xffff)] = stringarray,
 [((int) (D_T_FMT) & 0xffff)] = string,
 [((int) (D_FMT) & 0xffff)] = string,
 [((int) (T_FMT) & 0xffff)] = string,
 [((int) (T_FMT_AMPM) & 0xffff)] = string,
 [((int) (ERA) & 0xffff)] = stringlist,
 [((int) (ERA_YEAR) & 0xffff)] = string,
 [((int) (ERA_D_FMT) & 0xffff)] = string,
 [((int) (ALT_DIGITS) & 0xffff)] = stringlist,
 [((int) (ERA_D_T_FMT) & 0xffff)] = string,
 [((int) (ERA_T_FMT) & 0xffff)] = string,
 [((int) (_NL_TIME_ERA_NUM_ENTRIES) & 0xffff)] = word,
 [((int) (_NL_TIME_ERA_ENTRIES) & 0xffff)] = string,
 [((int) (_NL_WABDAY_1) & 0xffff)] = wstringarray,
 [((int) (_NL_WDAY_1) & 0xffff)] = wstringarray,
 [((int) (_NL_WABMON_1) & 0xffff)] = wstringarray,
 [((int) (_NL_WMON_1) & 0xffff)] = wstringarray,
 [((int) (_NL_WAM_STR) & 0xffff)] = wstringarray,
 [((int) (_NL_WD_T_FMT) & 0xffff)] = wstring,
 [((int) (_NL_WD_FMT) & 0xffff)] = wstring,
 [((int) (_NL_WT_FMT) & 0xffff)] = wstring,
 [((int) (_NL_WT_FMT_AMPM) & 0xffff)] = wstring,
 [((int) (_NL_WERA_YEAR) & 0xffff)] = wstring,
 [((int) (_NL_WERA_D_FMT) & 0xffff)] = wstring,
 [((int) (_NL_WALT_DIGITS) & 0xffff)] = wstringlist,
 [((int) (_NL_WERA_D_T_FMT) & 0xffff)] = wstring,
 [((int) (_NL_WERA_T_FMT) & 0xffff)] = wstring,
 [((int) (_NL_TIME_WEEK_NDAYS) & 0xffff)] = byte,
 [((int) (_NL_TIME_WEEK_1STDAY) & 0xffff)] = word,
 [((int) (_NL_TIME_WEEK_1STWEEK) & 0xffff)] = byte,
 [((int) (_NL_TIME_FIRST_WEEKDAY) & 0xffff)] = byte,
 [((int) (_NL_TIME_FIRST_WORKDAY) & 0xffff)] = byte,
 [((int) (_NL_TIME_CAL_DIRECTION) & 0xffff)] = byte,
 [((int) (_NL_TIME_TIMEZONE) & 0xffff)] = string,
 [((int) (_DATE_FMT) & 0xffff)] = string,
 [((int) (_NL_W_DATE_FMT) & 0xffff)] = wstring,
 [((int) (_NL_TIME_CODESET) & 0xffff)] = string,
 [((int) (ALTMON_1) & 0xffff)] = stringarray,       /* I'm going to add this */
 [((int) (_NL_WALTMON_1) & 0xffff)] = wstringarray, /* I'm going to add this */
};

We have ABDAY_1 = (((2) << 16) | (0)) so ((int) (ABDAY_1) & 0xffff) == 0;
let's expand it more:

static const enum value_type _nl_value_type_LC_TIME [] = {
 [0] = stringarray,
 [7] = stringarray,
 [14] = stringarray,
 [26] = stringarray,
 [38] = stringarray,
 [40] = string,
 [41] = string,
 [42] = string,
 [43] = string,
 [44] = stringlist,
 [45] = string,
 [46] = string,
 [47] = stringlist,
 [48] = string,
 [49] = string,
 [50] = word,
 [51] = string,
 [52] = wstringarray,
 [59] = wstringarray,
 [66] = wstringarray,
 [78] = wstringarray,
 [90] = wstringarray,
 [92] = wstring,
 [93] = wstring,
 [94] = wstring,
 [95] = wstring,
 [96] = wstring,
 [97] = wstring,
 [98] = wstringlist,
 [99] = wstring,
 [100] = wstring,
 [101] = byte,
 [102] = word,
 [103] = byte,
 [104] = byte,
 [105] = byte,
 [106] = byte,
 [107] = string,
 [108] = string,
 [109] = wstring,
 [110] = string,       /* _NL_TIME_CODESET */
 [111] = stringarray,  /* ALTMON_1 - I'm going to add this */
 [123] = wstringarray, /* _NL_WALTMON_1 - I'm going to add this */
};

The problem arises in CATTEST macro which ensures that:

	  if (cnt >= (sizeof (_nl_value_type_LC_##cat)		\
		      / sizeof (_nl_value_type_LC_##cat[0])))	\
	    goto puntdata;					\

so if cnt is larger than or equal to the number of elements in
_nl_value_type_LC_TIME array then whole locale category is rejected.
Old version [2] raised an assertion here. The array above has 124
elements while we need 135 (_NL_WALTMON_12 will expand to 134). This
is fixed if we declare a fixed size of the array which equals
_NL_NUM_LC_TIME.

A similar problem is in LC_CTYPE category, it ends with a subarray of
_NL_CTYPE_EXTRA_MAP_1,..,_NL_CTYPE_EXTRA_MAP_14. Probably this is
the reason why the size of this category is not checked in loadlocale.c.

I'm not telling if this is the solution to bug 19084 [3] but I will
need this fix soon. I thought it might be worth to post it already in
case someone else had the same problem in another context and might
need this patch before 10871 is fixed. No problem if you don't want to
treat this one-liner as a separate patch and merge it (silently?) into
the final solution of 10871. Also I will appreciate if you suggest me
a better solution of this problem.

Best regards,

Rafal Luzynski

PS. Some months ago I had another solution for 10871 which had one more
field after _NL_WALTMON_1..12 therefore the problem did not occur.


---
Links:

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=10871
[2]
https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commit;h=0062ace2292effc4135c15ea99b1931fea5e0203
[3] https://sourceware.org/bugzilla/show_bug.cgi?id=19084
  

thanks, your explanation clears things up.  your patch isn't related to
bug 19084 though as we discussed.  i'm fine with this patch then.  how
about this commit message though:

When constructing the locale arrays, we use categories.def with macros.
For stringarrays (like ABDAY_1), we end up only populating the first of
the array and leave holes for the rest (i.e. we omit ABDAY_2, etc...).
When there happen to be more items after it, things work out because we
implicitly pad out the array to cover the omitted elements.  However,
if these elements are specified last, we don't pad things out, and the
internal checks reject the compiled locale.

For example, if ABDAY_1 came last, the array would look like:
static const enum value_type _nl_value_type_LC_TIME[] = {
    ...
    [...ABDAY_1...] = stringarray,
};
Trying to index ABDAY_2 would now run off the end of the array.
-mike
  

Hi Mike,

10.04.2016 o 06:28 Mike Frysinger <vapier@gentoo.org> wrote:
>
>
> thanks, your explanation clears things up. your patch isn't related to
> bug 19084 though as we discussed. i'm fine with this patch then. how
> about this commit message though:
>
> When constructing the locale arrays, we use categories.def with macros.
> For stringarrays (like ABDAY_1), we end up only populating the first of
> the array and leave holes for the rest (i.e. we omit ABDAY_2, etc...).
> When there happen to be more items after it, things work out because we
> implicitly pad out the array to cover the omitted elements. However,
> if these elements are specified last, we don't pad things out, and the
> internal checks reject the compiled locale.
>
> For example, if ABDAY_1 came last, the array would look like:
> static const enum value_type _nl_value_type_LC_TIME[] = {
> ...
> [...ABDAY_1...] = stringarray,
> };
> Trying to index ABDAY_2 would now run off the end of the array.
> -mike

Yes, I agree with this.

A legal notice: I have not (yet!) signed any copyright assignment with
FSF but such a short update is not considered as legally significant [1]
so does not need any assignment. Also, if in doubt, here I state that
I will not complain if you (or any other person) commit, reject, change,
or do anything you find appropriate with this change.

By the way, do you think that after this patch you can stop skipping
the array size test for LC_TYPE? I wrote:

> A similar problem is in LC_CTYPE category, it ends with a subarray of
> _NL_CTYPE_EXTRA_MAP_1,..,_NL_CTYPE_EXTRA_MAP_14. Probably this is
> the reason why the size of this category is not checked in loadlocale.c.
[2]

Unless I'm wrong it may happen that a special case test for LC_CTYPE [3]
will no longer be necessary. I have no patch for this but the change would
be trivial, would you consider providing it yourself?

Best regards,

Rafal

[1] http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant
[2] https://sourceware.org/ml/libc-alpha/2016-03/msg00557.html
[3] https://sourceware.org/git/?p=glibc.git;a=blob;f=locale/loadlocale.c#l117
  

On 11 Apr 2016 21:35, Rafal Luzynski wrote:
> 10.04.2016 o 06:28 Mike Frysinger wrote:
> > thanks, your explanation clears things up. your patch isn't related to
> > bug 19084 though as we discussed. i'm fine with this patch then. how
> > about this commit message though:
> >
> > When constructing the locale arrays, we use categories.def with macros.
> > For stringarrays (like ABDAY_1), we end up only populating the first of
> > the array and leave holes for the rest (i.e. we omit ABDAY_2, etc...).
> > When there happen to be more items after it, things work out because we
> > implicitly pad out the array to cover the omitted elements. However,
> > if these elements are specified last, we don't pad things out, and the
> > internal checks reject the compiled locale.
> >
> > For example, if ABDAY_1 came last, the array would look like:
> > static const enum value_type _nl_value_type_LC_TIME[] = {
> > ...
> > [...ABDAY_1...] = stringarray,
> > };
> > Trying to index ABDAY_2 would now run off the end of the array.
> 
> Yes, I agree with this.
> 
> A legal notice: I have not (yet!) signed any copyright assignment with
> FSF but such a short update is not considered as legally significant [1]
> so does not need any assignment. Also, if in doubt, here I state that
> I will not complain if you (or any other person) commit, reject, change,
> or do anything you find appropriate with this change.

ok, for this less-than-one-line change, it's OK.  but for your other
patches (abmon stuff), we'll need CLA papers in place.

> By the way, do you think that after this patch you can stop skipping
> the array size test for LC_TYPE? I wrote:
> 
> > A similar problem is in LC_CTYPE category, it ends with a subarray of
> > _NL_CTYPE_EXTRA_MAP_1,..,_NL_CTYPE_EXTRA_MAP_14. Probably this is
> > the reason why the size of this category is not checked in loadlocale.c.
> [2]
> 
> Unless I'm wrong it may happen that a special case test for LC_CTYPE [3]
> will no longer be necessary. I have no patch for this but the change would
> be trivial, would you consider providing it yourself?

it does seem like we should be able to drop that test.  i'll put together
a patch for it once it passes tests.

btw, this code comes from this bug report:
	https://sourceware.org/bugzilla/show_bug.cgi?id=356
and the analysis there seems to match your own.
-mike
  

11.04.2016 22:02 Mike Frysinger <vapier@gentoo.org> wrote:
>
>
> On 11 Apr 2016 21:35, Rafal Luzynski wrote:
> > [...]
> > A legal notice: I have not (yet!) signed any copyright assignment with
> > FSF but such a short update is not considered as legally significant [1]
> > so does not need any assignment. Also, if in doubt, here I state that
> > I will not complain if you (or any other person) commit, reject, change,
> > or do anything you find appropriate with this change.
>
> ok, for this less-than-one-line change, it's OK. but for your other
> patches (abmon stuff), we'll need CLA papers in place.

OK, it's on the top of my TODO list.

> > By the way, do you think that after this patch you can stop skipping
> > the array size test for LC_TYPE? I wrote:
> >
> > > A similar problem is in LC_CTYPE category, it ends with a subarray of
> > > _NL_CTYPE_EXTRA_MAP_1,..,_NL_CTYPE_EXTRA_MAP_14. Probably this is
> > > the reason why the size of this category is not checked in loadlocale.c.
> > [2]
> >
> > Unless I'm wrong it may happen that a special case test for LC_CTYPE [3]
> > will no longer be necessary. I have no patch for this but the change would
> > be trivial, would you consider providing it yourself?
>
> it does seem like we should be able to drop that test. i'll put together
> a patch for it once it passes tests.
>
> btw, this code comes from this bug report:
> https://sourceware.org/bugzilla/show_bug.cgi?id=356
> and the analysis there seems to match your own.
> -mike

That's great. A short question is: is _NL_CTYPE_EXTRA_MAP_<n> an open array
of unknown and unlimited size or is it of unknown size but never more than
14 elements (and if this limit ever changes in future it will be reflected
in _NL_NUM_LC_CTYPE)?  In the former case the idea will work, in the latter
we should leave a special case which omits verifying the size of LC_CTYPE.

Regards,

Rafal
  

Mike,

What is the status of my patch [1] and the discussion below? I think you
expressed some positive opinion about it. I don't insist on accepting
and pushing it but if you accept it then I'd like to know because then
I should remove this change from my patches against bug 10871 [2] [3].

Regards,

Rafal

[1] https://sourceware.org/ml/libc-alpha/2016-03/msg00521.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=10871
[3] https://sourceware.org/ml/libc-alpha/2016-03/msg00616.html


11.04.2016 22:02 Mike Frysinger <vapier@gentoo.org> wrote:
>
> On 11 Apr 2016 21:35, Rafal Luzynski wrote:
> > 10.04.2016 o 06:28 Mike Frysinger wrote:
> > > thanks, your explanation clears things up. your patch isn't related to
> > > bug 19084 though as we discussed. i'm fine with this patch then. how
> > > about this commit message though:
> > >
> > > When constructing the locale arrays, we use categories.def with macros.
> > > For stringarrays (like ABDAY_1), we end up only populating the first of
> > > the array and leave holes for the rest (i.e. we omit ABDAY_2, etc...).
> > > When there happen to be more items after it, things work out because we
> > > implicitly pad out the array to cover the omitted elements. However,
> > > if these elements are specified last, we don't pad things out, and the
> > > internal checks reject the compiled locale.
> > >
> > > For example, if ABDAY_1 came last, the array would look like:
> > > static const enum value_type _nl_value_type_LC_TIME[] = {
> > > ...
> > > [...ABDAY_1...] = stringarray,
> > > };
> > > Trying to index ABDAY_2 would now run off the end of the array.
> >
> > Yes, I agree with this.
> >
> > A legal notice: I have not (yet!) signed any copyright assignment with
> > FSF but such a short update is not considered as legally significant [1]
> > so does not need any assignment. Also, if in doubt, here I state that
> > I will not complain if you (or any other person) commit, reject, change,
> > or do anything you find appropriate with this change.
>
> ok, for this less-than-one-line change, it's OK. but for your other
> patches (abmon stuff), we'll need CLA papers in place.
>
> > By the way, do you think that after this patch you can stop skipping
> > the array size test for LC_TYPE? I wrote:
> >
> > > A similar problem is in LC_CTYPE category, it ends with a subarray of
> > > _NL_CTYPE_EXTRA_MAP_1,..,_NL_CTYPE_EXTRA_MAP_14. Probably this is
> > > the reason why the size of this category is not checked in loadlocale.c.
> > [2]
> >
> > Unless I'm wrong it may happen that a special case test for LC_CTYPE [3]
> > will no longer be necessary. I have no patch for this but the change would
> > be trivial, would you consider providing it yourself?
>
> it does seem like we should be able to drop that test. i'll put together
> a patch for it once it passes tests.
>
> btw, this code comes from this bug report:
> https://sourceware.org/bugzilla/show_bug.cgi?id=356
> and the analysis there seems to match your own.
> -mike