search locale archive again after alias expansion
diff mbox

Message ID oregpd19rz.fsf@livre.home
State New
Headers show

Commit Message

Alexandre Oliva Feb. 26, 2015, 6:12 a.m. UTC
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

Andreas Schwab Feb. 26, 2015, 9:40 a.m. UTC | #1
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.
Alexandre Oliva Feb. 26, 2015, 5:57 p.m. UTC | #2
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 ;-)
Carlos O'Donell Feb. 26, 2015, 9:44 p.m. UTC | #3
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.
Alexandre Oliva Feb. 27, 2015, 6:45 a.m. UTC | #4
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.
Andreas Schwab March 2, 2015, 8:21 a.m. UTC | #5
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.

Patch
diff mbox

diff --git a/locale/findlocale.c b/locale/findlocale.c
index 5e2639b..9e7df12 100644
--- a/locale/findlocale.c
+++ b/locale/findlocale.c
@@ -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: