On Thu, May 30, 2013 at 11:49 PM, Stefan Sperling <s...@openbsd.org> wrote:
> On Thu, May 30, 2013 at 05:18:48PM -0430, Andres Perera wrote:
>> As I mentioned, there's code that expects the prior layout, and that's
>> confusing.
>>
>> on src/lib/libc/locale/setlocale.c, load_locale_sub() :
>>
>> 228 len = snprintf(name, sizeof(name), "%s/%s/%s",
>> 229 _PATH_LOCALE, locname, categories[category]);
>> 230 if (len < 0 || len >= sizeof(name))
>> 231 return -1;
>
> Right, thanks for pointing this out.
>
> I think the above check can just be removed.
> It seems to serve no purpose other than making sure that the path
> constructed from _PATH_LOCALE and the locname argument doesn't
> exceed PATH_MAX. This is redundant because the same check is
> performed again within _xpg4_setrunelocale(). If we assume that functions
> handling other LC_* categories might use different paths in the future,
> it makes sense to perform this overflow check only inside of the
> LC_*-specific functions, rather than upfront.
Besides being redundant, it's generally better to bail on open() error
instead of checking if the length is less than PATH_MAX in
anticipation.
[Unrelated to your patch, but since we are on the subject of path
checks in setlocale()...]
However, since the routines also use locale data paths as keys for a
dynamically allocated cache of their contents, a single check should
persist. This raises an unrelated issue about the cache index: since
libc doesn't support arbitrary paths for locale files, there's no need
to index the cache by full locale data paths because all of the paths
are anchored at _PATH_LOCALE.
>
>> on src/lib/libc/locale/setrunelocale.c, _xpg4_setrunelocale():
>>
>> 184 len = snprintf(path, sizeof(path),
>> 185 "%s/%s/LC_CTYPE", _PATH_LOCALE, encoding);
>> 186 if (len < 0 || len >= sizeof(path))
>> 187 return ENAMETOOLONG;
>
> This section is modified as part of my diff, isn't it?
Sorry, this assertion remains correct after your diff so there was no
need for me to list it.
In any case, it's also redundant and should be zapped to prevent
further confusion. There's yet another (x < PATH_MAX) check--making it
the third one--which is a better candidate to keep due to its coupling
with the cache population logic:
125 int
126 _newrunelocale(const char *path)
127 {
128 struct localetable *lt;
129 FILE *fp;
130 _RuneLocale *rl;
131
132 if (strlen(path) + 1 > sizeof(lt->path))
133 return EINVAL;
>
>> > + /* Assume "<whatever>.<encoding>" locale name. */
>>
>> There should be some notion of syntax for cc_LL.CTYPE, even if only
>> mentioned in comments.
>>
>> E.g.,
>>
>> ISO 3166-1 for country codes and BCP 47 for language tags.
>>
>> glibc did not do this and directly because of that it's a mess to
>> navigate their structure.
>
> You mean we should specify encoding name syntax in a comment?
> If so, my answer would be that recognized encoding names are
> specified by filenames we use in /usr/share/locale. A comment
> documenting the same would risk becoming obsolete over time.
>
> Updated diff, with the redundant check removed:
[...]
