[PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Jakub Jelinek]

Hi!

The following patch fixes a bunch of things.

Primarily, addition of __names[] array to __locale_struct broke binary
compatibility - programs using e.g. isdigit_l etc. suddenly give
broken results (including libstdc++-v3).

Second, at least tllocale.ps documents uselocale:
> The function returns the locale handle previously passed
> to uselocale() for the current thread. If there was no such call
> before the return value is LC_GLOBAL_LOCALE.
That's also consistent with setlocale behaviour (and libstdc++-v3 uses it).
But current uselocale only returns previous locale if called with
uselocale(NULL) and otherwise returns the argument which was passed to it.
That would mean
locale_t old = uselocale(new);
something
uselocale(old);
sequence would have to be rewritten as:
locale_t old = uselocale(NULL);
uselocale(new);
something
uselocale(old);
I really thing we should follow the paper in here.

Third thing is uselocale and gettext - although __names was introduced,
newlocale created locales would have all __names[X] set to _nl_C_name
and thus gettext would not work properly.

The fourth thing is newlocale/duplocale failure handling and on success
avoiding memory leaking.

2002-08-31  Jakub Jelinek  <jakub@redhat.com>

	* locale/newlocale.c: Include string.h.
	(__newlocale): Fill in __names elements, free them on failure.
	On failure call _nl_remove_locale when necessary.
	Free old __names and all locale data if using base.
	* locale/duplocale.c (__duplocale): Free newly strduped __names,
	not members of dataset on failure.
	* locale/uselocale.c (__uselocale): Always return previous locale_t,
	not only when newloc is NULL.
	* locale/xlocale.h (__struct locale_struct): Put __names last
	for binary compatibility of inline is*_l and to*_l.

--- libc/locale/newlocale.c.jj	2002-08-12 15:27:48.000000000 +0200
+++ libc/locale/newlocale.c	2002-08-31 17:06:46.000000000 +0200
@@ -22,6 +22,7 @@
 #include <errno.h>
 #include <locale.h>
 #include <stdlib.h>
+#include <string.h>
 
 #include "localeinfo.h"
 
@@ -148,7 +149,32 @@ __newlocale (int category_mask, const ch
 	result.__locales[cnt] = _nl_find_locale (locale_path, locale_path_len,
 						 cnt, &newnames[cnt]);
 	if (result.__locales[cnt] == NULL)
-	  return NULL;
+	  {
+ exit_free:
+	    for (--cnt; cnt >= 0; --cnt)
+	      if (cnt != LC_ALL && (category_mask & 1 << cnt) != 0)
+		{
+		  if (result.__locales[cnt]->usage_count != UNDELETABLE)
+		    /* We can remove the data.  */
+		    _nl_remove_locale (cnt, result.__locales[cnt]);
+		  if (result.__names[cnt] != _nl_C_name)
+		    free ((char *) result.__names[cnt]);
+		}
+	    return NULL;
+	  }
+	if (newnames[cnt] == _nl_C_name)
+	  result.__names[cnt] = _nl_C_name;
+	else
+	  {
+	    result.__names[cnt] = __strdup (newnames[cnt]);
+	    if (result.__names[cnt] == NULL)
+	      {
+		if (result.__locales[cnt]->usage_count != UNDELETABLE)
+		  /* We can remove the data.  */
+		  _nl_remove_locale (cnt, result.__locales[cnt]);
+		goto exit_free;
+	      }
+	  }
       }
 
   /* We successfully loaded all required data.  */
@@ -157,12 +183,24 @@ __newlocale (int category_mask, const ch
       /* Allocate new structure.  */
       result_ptr = (__locale_t) malloc (sizeof (struct __locale_struct));
       if (result_ptr == NULL)
-	return NULL;
-
+	goto exit_free;
     }
   else
-    /* We modify the base structure.  */
-    result_ptr = base;
+    {
+      /* We modify the base structure.  */
+      result_ptr = base;
+
+      /* Need to free the old names and old locale data.  */
+      for (cnt = 0; cnt < __LC_LAST; ++cnt)
+	if (cnt != LC_ALL && (category_mask & 1 << cnt) != 0)
+	  {
+	    if (base->__locales[cnt]->usage_count != UNDELETABLE)
+	      /* We can remove the data.  */
+	      _nl_remove_locale (cnt, base->__locales[cnt]);
+	    if (base->__names[cnt] != _nl_C_name)
+	      free ((char *) base->__names[cnt]);
+	  }
+    }
 
   *result_ptr = result;
 
--- libc/locale/duplocale.c.jj	2002-08-31 09:40:42.000000000 +0200
+++ libc/locale/duplocale.c	2002-08-31 17:06:46.000000000 +0200
@@ -55,9 +55,9 @@ __duplocale (__locale_t dataset)
 	      result->__names[cnt] = __strdup (dataset->__names[cnt]);
 	      if (result->__names[cnt] == NULL)
 		{
-		  while (cnt-- > 0)
-		    if (dataset->__names[cnt] != _nl_C_name)
-		      free ((char *) dataset->__names[cnt]);
+		  while (--cnt >= 0)
+		    if (result->__names[cnt] != _nl_C_name)
+		      free ((char *) result->__names[cnt]);
 		  free (result);
 		  result = NULL;
 		  break;
--- libc/locale/uselocale.c.jj	2002-08-28 12:58:05.000000000 +0200
+++ libc/locale/uselocale.c	2002-08-31 18:07:09.000000000 +0200
@@ -28,16 +28,16 @@
 locale_t
 __uselocale (locale_t newloc)
 {
-  if (newloc == NULL)
-    {
-      locale_t loc = __libc_tsd_get (LOCALE);
-      return loc == &_nl_global_locale ? LC_GLOBAL_LOCALE : loc;
-    }
-  else
+  locale_t oldloc = __libc_tsd_get (LOCALE);
+
+  if (oldloc == &_nl_global_locale)
+    oldloc = LC_GLOBAL_LOCALE;
+
+  if (newloc != NULL)
     {
-      const locale_t locobj
-	= newloc == LC_GLOBAL_LOCALE ? &_nl_global_locale : newloc;
-      __libc_tsd_set (LOCALE, locobj);
+      if (newloc == LC_GLOBAL_LOCALE)
+	newloc = &_nl_global_locale;
+      __libc_tsd_set (LOCALE, newloc);
 
 #ifdef NL_CURRENT_INDIRECT
       /* Now we must update all the per-category thread-local variables to
@@ -58,13 +58,13 @@ __uselocale (locale_t newloc)
 	weak_extern (_nl_current_##category##_used)			      \
 	weak_extern (_nl_current_##category)				      \
 	if (&_nl_current_##category##_used != 0)			      \
-	  _nl_current_##category = &locobj->__locales[category];	      \
+	  _nl_current_##category = &newloc->__locales[category];	      \
       }
 # include "categories.def"
 # undef	DEFINE_CATEGORY
 #endif
     }
 
-  return newloc;
+  return oldloc;
 }
 weak_alias (__uselocale, uselocale)
--- libc/locale/xlocale.h.jj	2002-08-31 09:40:42.000000000 +0200
+++ libc/locale/xlocale.h	2002-08-31 19:40:35.000000000 +0200
@@ -29,12 +29,14 @@ typedef struct __locale_struct
 {
   /* Note: LC_ALL is not a valid index into this array.  */
   struct locale_data *__locales[13]; /* 13 = __LC_LAST. */
-  const char *__names[13];
 
   /* To increase the speed of this solution we add some special members.  */
   const unsigned short int *__ctype_b;
   const int *__ctype_tolower;
   const int *__ctype_toupper;
+
+  /* Note: LC_ALL is not a valid index into this array.  */
+  const char *__names[13]; /* 13 = __LC_LAST. */
 } *__locale_t;
 
 #endif /* xlocale.h */

	Jakub

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Roland McGrath]

D'oh!  I had of course made the newlocale changes but failed to check
that file in, I don't know how I failed to notice it there in my tree.
The uselocale behavior was an oversight and of course duplocale was a typo.
I don't like uglifying the natural order of the struct declaration for
binary compatibility with a release that has never been.

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Jakub Jelinek]

On Sat, Aug 31, 2002 at 11:29:43AM -0700, Roland McGrath wrote:
> I don't like uglifying the natural order of the struct declaration for
> binary compatibility with a release that has never been.

Well, libstdc++ 3.1/2 certainly uses them for at least 8 months now,
and is*_l has been around for a couple of years.

Anyway, if you think it is possible to reorder the struct (and
thus force recompilation of all C++ 3.2 programs - __ctype_* fields
are accessed from /usr/include/c++/3.*/ headers, so it isn't just
libstdc++ recompilation which is necessary), __ctype_ fields
should be certainly at the beginning so that __LC_LAST can grow.

	Jakub

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jakub Jelinek wrote:

> Well, libstdc++ 3.1/2 certainly uses them for at least 8 months now,
> and is*_l has been around for a couple of years.

Right, this is a binary compatibility issues.  Jakub's patch is correct. 
  libstdc++ is a legitimate user of the _l functions for some time.

> 
> Anyway, if you think it is possible to reorder the struct (and
> thus force recompilation of all C++ 3.2 programs - __ctype_* fields
> are accessed from /usr/include/c++/3.*/ headers, so it isn't just
> libstdc++ recompilation which is necessary), __ctype_ fields
> should be certainly at the beginning so that __LC_LAST can grow.

I don't expect any growth.  Let's stick with the patch.

- -- 
- ---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9cRCC2ijCOnn/RHQRAsvlAJ9KXSLlUULqpHFhXXAJ/uV54dwrcgCfZ8fL
swHMwlFnTfx9+5v3nCS0FP0=
=oGJ/
-----END PGP SIGNATURE-----

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Roland McGrath]

Oh, I hadn't realized that *_l had been there in 2.2.  That we stay
compatible with.  If it had just been the versions base on 2.3 devel state
by RH or whoever, those are not releases that count for us.

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Jakub Jelinek]

On Sat, Aug 31, 2002 at 11:29:43AM -0700, Roland McGrath wrote:
> D'oh!  I had of course made the newlocale changes but failed to check
> that file in, I don't know how I failed to notice it there in my tree.

I see a few problems with your newlocale.c though
(and I think my version is shorter and had these issues sorted out,
though more eyes see more...).

  if (base == NULL)
    {
      /* Allocate new structure.  */
      result_ptr = (__locale_t) malloc (sizeof (struct __locale_struct));
      if (result_ptr == NULL)
        return NULL;
_nl_remove_locale is not called in this case, leaking locale data.

      /* Install strdup'd names in the new structure's __names array.  */
      for (cnt = 0; cnt < __LC_LAST; ++cnt)
        if (cnt != LC_ALL && result.__names[cnt] != _nl_C_name)
			     ^^^^^^^^^^^^^^^^^^^
As base is NULL, all result.__names[x] are _nl_C_name and nobody changed
it yet, so this loop does nothing. Should be newnames[cnt].
          {
            result.__names[cnt] = __strdup (((category_mask & 1 << cnt) !=  0)
                                            ? newnames[cnt]
                                            : result.__names[cnt]);
This is unnecessary, if cnt is not in category_mark, it will surely be a
_nl_C_name.
            if (result.__names[cnt] == NULL)
              {
                while (cnt-- > 0)
                  if (result.__names[cnt] != _nl_C_name)
                    free ((char *) result.__names[cnt]);
_nl_remove_locale is not called in this case, leaking locale data.
                free (result_ptr);
                return NULL;
              }
          }
    }
  else
    {
      /* We modify the base structure.
         First strdup the names we were given for the new locale.  */

      for (cnt = 0; cnt < __LC_LAST; ++cnt)
        if (cnt != LC_ALL && ((category_mask & 1 << cnt) != 0)
            && result.__names[cnt] != _nl_C_name)
Again, this should probably be newnames[cnt].
          {
            newnames[cnt] = __strdup (newnames[cnt]);
            if (newnames[cnt] == NULL)
              {
                while (cnt-- > 0)
                  if (newnames[cnt] != NULL)
And this should test _nl_C_name instead.
                    free ((char *) newnames[cnt]);
                return NULL;
_nl_remove_locale is not called in this case, leaking locale data.
              }
          }
        else
          newnames[cnt] = NULL;

      /* Now that we can't lose, install the new names.  */
      for (cnt = 0; cnt < __LC_LAST; ++cnt)
        if (newnames[cnt] != NULL)
          {
            if (result.__names[cnt] != _nl_C_name)
              free ((char *) result.__names[cnt]);
            result.__names[cnt] = __strdup (newnames[cnt]);
strdup of strdup?
          }

      result_ptr = base;


	Jakub

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Roland McGrath]

I have put in similar changes now that I think are right.  Please check them.
We certainly need some tests for this stuff if anybody feels like writing some.

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jakub Jelinek wrote:

> strdup of strdup?

And while we are at it: the various strdups should be collapsed.  The 
individual memory allocations are rather expensive.  Just collect the 
sizes of the names while iterating over the locales and allocate one 
chunk of memory.  The size doesn't have to be exact.  When newlocale is 
called with base != NULL the memory block can be resized (if necessary).

- -- 
- ---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9cRV92ijCOnn/RHQRAhjUAKDGub9gLgGtYgdK4vTU7HrjLHZgPACfdQ9J
ZpTfX4ALAh50Zy61uhT3v2E=
=/6Qw
-----END PGP SIGNATURE-----

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Roland McGrath]

> And while we are at it: the various strdups should be collapsed.  The 
> individual memory allocations are rather expensive.  Just collect the 
> sizes of the names while iterating over the locales and allocate one 
> chunk of memory.  The size doesn't have to be exact.  When newlocale is 
> called with base != NULL the memory block can be resized (if necessary).

Ok, will do.

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Jakub Jelinek]

On Sat, Aug 31, 2002 at 12:04:15PM -0700, Roland McGrath wrote:
> I have put in similar changes now that I think are right.  Please check them.
> We certainly need some tests for this stuff if anybody feels like writing some.

          free_data_and_exit:
            while (cnt-- > 0)
              if (((category_mask & 1 << cnt) != 0)
...
                while (cnt-- > 0)
                  if (result.__names[cnt] != _nl_C_name)
                    free ((char *) result.__names[cnt]);
                goto free_data_and_exit;
cnt will be zero here, so free_data_and_exit will not do its job.

	Jakub

Re: [PATCH] Fix newlocale, uselocale, duplocale, is*_l and to*_l

[Roland McGrath]

Ok, now I think I got it all right.  But I will have to go eat before
I am thinking real clearly.

Note that newlocale always has to do a new alloc and free the old later
instead of realloc, so the name table can go from "short\0foo\0" to
"longer\0foo\0" without clobbering "foo" before you get to it.