search locale archive again after alias expansion
Commit Message
Here's a follow-up patch that gets us rid of all the const-casting in
loc_name and *name. This ensures we won't write to stuff that should be
const by accident, and avoids unsafely dereferencing pointers to
pointers.
Ok to install?
for ChangeLog
[BZ #15969]
* locale/findlocale.c (_nl_find_locale): Introduce const
version of loc_name and drop unsafe type casts.
---
locale/findlocale.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
Comments
Alexandre Oliva <aoliva@redhat.com> writes:
> - loc_name = strdupa (loc_name);
> + char *loc_name = strdupa (cloc_name);
If you rename this to loc_name_copy, instead of the other way round, you
get nicer variable names, and a smaller delta as a bonus.
Andreas.
On Feb 26, 2015, Andreas Schwab <schwab@suse.de> wrote:
> Alexandre Oliva <aoliva@redhat.com> writes:
>> - loc_name = strdupa (loc_name);
>> + char *loc_name = strdupa (cloc_name);
> If you rename this to loc_name_copy, instead of the other way round, you
> get nicer variable names, and a smaller delta as a bonus.
Yeah, but that wouldn't have made it crystal clear that no uses of the
original loc_name remained in the affected range ;-)
On 02/26/2015 01:12 AM, Alexandre Oliva wrote:
> Here's a follow-up patch that gets us rid of all the const-casting in
> loc_name and *name. This ensures we won't write to stuff that should be
> const by accident, and avoids unsafely dereferencing pointers to
> pointers.
>
> Ok to install?
Not OK, please make the patch minimal.
> for ChangeLog
>
> [BZ #15969]
> * locale/findlocale.c (_nl_find_locale): Introduce const
> version of loc_name and drop unsafe type casts.
The name change from loc_name to cloc_name makes backports to release
branches difficult as any patches applying on top of this one will
require this patch also.
Please checkin a minimal patch that simply changes the type to const
and fixes the warning.
OK to checkin a minimal patch.
Stylistically speaking we don't normally encode the const-ness into
the name of the variable, leaving that up to the compiler and linker/loader
to enforce as you try to write to that variable e.g. warnings or failures
as you try to write to RO-data.
Cheers,
Carlos.
On Feb 26, 2015, "Carlos O'Donell" <carlos@redhat.com> wrote:
> On 02/26/2015 01:12 AM, Alexandre Oliva wrote:
>> Here's a follow-up patch that gets us rid of all the const-casting in
>> loc_name and *name. This ensures we won't write to stuff that should be
>> const by accident, and avoids unsafely dereferencing pointers to
>> pointers.
>>
>> Ok to install?
> Not OK, please make the patch minimal.
Please elaborate. The minimal patch is already in, but it addresses a
different problem. It fixes only the warning, by introducing yet
another unsafe cast where we had plenty. This patch that you reject
intends to REMOVE the unsafe casts and solve the violations of C
aliasing rules that they cause.
>> [BZ #15969]
>> * locale/findlocale.c (_nl_find_locale): Introduce const
>> version of loc_name and drop unsafe type casts.
> The name change from loc_name to cloc_name makes backports to release
> branches difficult as any patches applying on top of this one will
> require this patch also.
What will make backports difficult is the removal of the casts. Those
occur at every few lines in each hunk; no context diff that touched
lines containing the const char* variable would avoid having at least
one line that currently contains a faulty cast.
We need a const char* variable to avoid the casts and potential
violation of alias safety rules (writing a char const* to a char *
lvalue is not safe), and we need a char * variable after the copy,
because we write to the string. They can't both have the same name,
unless they were in different scopes, which would require reindenting,
making the differences even bigger.
Now, if I were to rename the non-const version, as Andreas suggested,
this would *increase*, rather than decrease, the patch conflict surface
area, because we'd have conflicts due to the dropped casts, in the area
where I renamed the variable, and more conflicts due to the use of the
renamed non-const char * variable after we copy the const string to a
writable region.
Alexandre Oliva <aoliva@redhat.com> writes:
> On Feb 26, 2015, Andreas Schwab <schwab@suse.de> wrote:
>
>> Alexandre Oliva <aoliva@redhat.com> writes:
>>> - loc_name = strdupa (loc_name);
>>> + char *loc_name = strdupa (cloc_name);
>
>> If you rename this to loc_name_copy, instead of the other way round, you
>> get nicer variable names, and a smaller delta as a bonus.
>
> Yeah, but that wouldn't have made it crystal clear that no uses of the
> original loc_name remained in the affected range ;-)
That wouldn't work either way.
Andreas.
@@ -105,7 +105,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len,
{
int mask;
/* Name of the locale for this category. */
- char *loc_name = (char *) *name;
+ const char *cloc_name = *name;
const char *language;
const char *modifier;
const char *territory;
@@ -113,39 +113,39 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len,
const char *normalized_codeset;
struct loaded_l10nfile *locale_file;
- if (loc_name[0] == '\0')
+ if (cloc_name[0] == '\0')
{
/* The user decides which locale to use by setting environment
variables. */
- loc_name = getenv ("LC_ALL");
- if (!name_present (loc_name))
- loc_name = getenv (_nl_category_names.str
- + _nl_category_name_idxs[category]);
- if (!name_present (loc_name))
- loc_name = getenv ("LANG");
- if (!name_present (loc_name))
- loc_name = (char *) _nl_C_name;
+ cloc_name = getenv ("LC_ALL");
+ if (!name_present (cloc_name))
+ cloc_name = getenv (_nl_category_names.str
+ + _nl_category_name_idxs[category]);
+ if (!name_present (cloc_name))
+ cloc_name = getenv ("LANG");
+ if (!name_present (cloc_name))
+ cloc_name = _nl_C_name;
}
/* We used to fall back to the C locale if the name contains a slash
character '/', but we now check for directory traversal in
valid_locale_name, so this is no longer necessary. */
- if (__builtin_expect (strcmp (loc_name, _nl_C_name), 1) == 0
- || __builtin_expect (strcmp (loc_name, _nl_POSIX_name), 1) == 0)
+ if (__builtin_expect (strcmp (cloc_name, _nl_C_name), 1) == 0
+ || __builtin_expect (strcmp (cloc_name, _nl_POSIX_name), 1) == 0)
{
/* We need not load anything. The needed data is contained in
the library itself. */
- *name = (char *) _nl_C_name;
+ *name = _nl_C_name;
return _nl_C[category];
}
- else if (!valid_locale_name (loc_name))
+ else if (!valid_locale_name (cloc_name))
{
__set_errno (EINVAL);
return NULL;
}
- *name = loc_name;
+ *name = cloc_name;
/* We really have to load some data. First we try the archive,
but only if there was no LOCPATH environment variable specified. */
@@ -158,11 +158,10 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len,
/* Nothing in the archive with the given name. Expanding it as
an alias and retry. */
- loc_name = (char *) _nl_expand_alias (*name);
- if (loc_name != NULL)
+ cloc_name = _nl_expand_alias (*name);
+ if (cloc_name != NULL)
{
- data = _nl_load_locale_from_archive (category,
- (const char **) &loc_name);
+ data = _nl_load_locale_from_archive (category, &cloc_name);
if (__builtin_expect (data != NULL, 1))
return data;
}
@@ -175,14 +174,14 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len,
/* We really have to load some data. First see whether the name is
an alias. Please note that this makes it impossible to have "C"
or "POSIX" as aliases. */
- loc_name = (char *) _nl_expand_alias (*name);
+ cloc_name = _nl_expand_alias (*name);
- if (loc_name == NULL)
+ if (cloc_name == NULL)
/* It is no alias. */
- loc_name = (char *) *name;
+ cloc_name = *name;
/* Make a writable copy of the locale name. */
- loc_name = strdupa (loc_name);
+ char *loc_name = strdupa (cloc_name);
/* LOCALE can consist of up to four recognized parts for the XPG syntax: