[google] Handle NULL return values in setlocale calls

[Diego Novillo, 2011-04-16 20:21:10]

I'm committing this patch for Jing Yu on google/main.

The patch handles NULL values returned from setlocale.  Jing, could
you please describe why this was needed?  Is this a patch that you
will want to submit for trunk?

Tested on x86_64.  Committed to google/main.


Diego.

2011-04-15  Jing Yu  <jingyu@google.com>
    
	Google ref 46499.

    	* config/locale/generic/c_locale.cc (__convert_to_v): Handle
    	NULL return value from setlocale.
    	* config/locale/generic/c_locale.h (__convert_from_v): Likewise.
    	* config/locale/generic/time_members.cc (_M_put): Likewise.

diff --git a/libstdc++-v3/config/locale/generic/c_locale.cc
b/libstdc++-v3/config/locale/generic/c_locale.cc
index fb9b425..7e34fcf 100644
--- a/libstdc++-v3/config/locale/generic/c_locale.cc
+++ b/libstdc++-v3/config/locale/generic/c_locale.cc
@@ -52,10 +52,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       // Assumes __s formatted for "C" locale.
       char* __old = setlocale(LC_ALL, 0);
-      const size_t __len = strlen(__old) + 1;
-      char* __sav = new char[__len];
-      memcpy(__sav, __old, __len);
-      setlocale(LC_ALL, "C");
+      char* __sav = NULL;
+      if (__old != NULL)
+        {
+          const size_t __len = strlen(__old) + 1;
+          __sav = new char[__len];
+          memcpy(__sav, __old, __len);
+          setlocale(LC_ALL, "C");
+        }
       char* __sanity;
       bool __overflow = false;
 
@@ -117,10 +121,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       // Assumes __s formatted for "C" locale.
       char* __old = setlocale(LC_ALL, 0);
-      const size_t __len = strlen(__old) + 1;
-      char* __sav = new char[__len];
-      memcpy(__sav, __old, __len);
-      setlocale(LC_ALL, "C");
+      char* __sav = NULL;
+      if (__old != NULL)
+        {
+          const size_t __len = strlen(__old) + 1;
+          __sav = new char[__len];
+          memcpy(__sav, __old, __len);
+          setlocale(LC_ALL, "C");
+        }
       char* __sanity;
 
 #if !__DBL_HAS_INFINITY__
@@ -162,10 +170,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       // Assumes __s formatted for "C" locale.
       char* __old = setlocale(LC_ALL, 0);
-      const size_t __len = strlen(__old) + 1;
-      char* __sav = new char[__len];
-      memcpy(__sav, __old, __len);
-      setlocale(LC_ALL, "C");
+      char* __sav = NULL;
+      if (__old != NULL)
+        {
+          const size_t __len = strlen(__old) + 1;
+          __sav = new char[__len];
+          memcpy(__sav, __old, __len);
+          setlocale(LC_ALL, "C");
+        }
 
 #if !__LDBL_HAS_INFINITY__
       errno = 0;
diff --git a/libstdc++-v3/config/locale/generic/c_locale.h
b/libstdc++-v3/config/locale/generic/c_locale.h
index 2c76000..6e6f673 100644
--- a/libstdc++-v3/config/locale/generic/c_locale.h
+++ b/libstdc++-v3/config/locale/generic/c_locale.h
@@ -60,7 +60,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
     char* __old = std::setlocale(LC_NUMERIC, 0);
     char* __sav = 0;
-    if (__builtin_strcmp(__old, "C"))
+    if (__old != NULL && __builtin_strcmp(__old, "C"))
       {
 	const size_t __len = __builtin_strlen(__old) + 1;
 	__sav = new char[__len];
diff --git a/libstdc++-v3/config/locale/generic/time_members.cc
b/libstdc++-v3/config/locale/generic/time_members.cc
index 3031075..fb9eb6e 100644
--- a/libstdc++-v3/config/locale/generic/time_members.cc
+++ b/libstdc++-v3/config/locale/generic/time_members.cc
@@ -45,10 +45,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   const tm* __tm) const throw()
     {
       char* __old = setlocale(LC_ALL, 0);
-      const size_t __llen = strlen(__old) + 1;
-      char* __sav = new char[__llen];
-      memcpy(__sav, __old, __llen);
-      setlocale(LC_ALL, _M_name_timepunct);
+      char* __sav = NULL;
+      if (__old != NULL)
+        {
+          const size_t __llen = strlen(__old) + 1;
+          __sav = new char[__llen];
+          memcpy(__sav, __old, __llen);
+          setlocale(LC_ALL, _M_name_timepunct);
+        }
       const size_t __len = strftime(__s, __maxlen, __format, __tm);
       setlocale(LC_ALL, __sav);
       delete [] __sav;
@@ -130,10 +134,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   const tm* __tm) const throw()
     {
       char* __old = setlocale(LC_ALL, 0);
-      const size_t __llen = strlen(__old) + 1;
-      char* __sav = new char[__llen];
-      memcpy(__sav, __old, __llen);
-      setlocale(LC_ALL, _M_name_timepunct);
+      char* __sav = NULL;
+      if (__old != NULL)
+        {
+          const size_t __llen = strlen(__old) + 1;
+          __sav = new char[__llen];
+          memcpy(__sav, __old, __llen);
+          setlocale(LC_ALL, _M_name_timepunct);
+        }
       const size_t __len = wcsftime(__s, __maxlen, __format, __tm);
       setlocale(LC_ALL, __sav);
       delete [] __sav;


--
This patch is available for review at http://codereview.appspot.com/4444046

[jingyu, 2011-04-17 01:18:01]

Thanks Diego for committing this patch.

Yes, I would like to submit the patch to trunk. The reason is for this
patch is that setlocale() in bionicC always returns NULL (bionicC does
not support setlocale()), however libstdc++ does not handle the NULL
return value of setlocale(xxx,NULL) and thus causes segmentation
fault.

Thanks,
Jing

On Sat, Apr 16, 2011 at 1:20 PM, Diego Novillo <dnovillo@google.com> wrote:
> I'm committing this patch for Jing Yu on google/main.
>
> The patch handles NULL values returned from setlocale.  Jing, could
> you please describe why this was needed?  Is this a patch that you
> will want to submit for trunk?
>
> Tested on x86_64.  Committed to google/main.
>
>
> Diego.
>
> 2011-04-15  Jing Yu  <jingyu@google.com>
>
>        Google ref 46499.
>
>        * config/locale/generic/c_locale.cc (__convert_to_v): Handle
>        NULL return value from setlocale.
>        * config/locale/generic/c_locale.h (__convert_from_v): Likewise.
>        * config/locale/generic/time_members.cc (_M_put): Likewise.
>
> diff --git a/libstdc++-v3/config/locale/generic/c_locale.cc
b/libstdc++-v3/config/locale/generic/c_locale.cc
> index fb9b425..7e34fcf 100644
> --- a/libstdc++-v3/config/locale/generic/c_locale.cc
> +++ b/libstdc++-v3/config/locale/generic/c_locale.cc
> @@ -52,10 +52,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     {
>       // Assumes __s formatted for "C" locale.
>       char* __old = setlocale(LC_ALL, 0);
> -      const size_t __len = strlen(__old) + 1;
> -      char* __sav = new char[__len];
> -      memcpy(__sav, __old, __len);
> -      setlocale(LC_ALL, "C");
> +      char* __sav = NULL;
> +      if (__old != NULL)
> +        {
> +          const size_t __len = strlen(__old) + 1;
> +          __sav = new char[__len];
> +          memcpy(__sav, __old, __len);
> +          setlocale(LC_ALL, "C");
> +        }
>       char* __sanity;
>       bool __overflow = false;
>
> @@ -117,10 +121,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     {
>       // Assumes __s formatted for "C" locale.
>       char* __old = setlocale(LC_ALL, 0);
> -      const size_t __len = strlen(__old) + 1;
> -      char* __sav = new char[__len];
> -      memcpy(__sav, __old, __len);
> -      setlocale(LC_ALL, "C");
> +      char* __sav = NULL;
> +      if (__old != NULL)
> +        {
> +          const size_t __len = strlen(__old) + 1;
> +          __sav = new char[__len];
> +          memcpy(__sav, __old, __len);
> +          setlocale(LC_ALL, "C");
> +        }
>       char* __sanity;
>
>  #if !__DBL_HAS_INFINITY__
> @@ -162,10 +170,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     {
>       // Assumes __s formatted for "C" locale.
>       char* __old = setlocale(LC_ALL, 0);
> -      const size_t __len = strlen(__old) + 1;
> -      char* __sav = new char[__len];
> -      memcpy(__sav, __old, __len);
> -      setlocale(LC_ALL, "C");
> +      char* __sav = NULL;
> +      if (__old != NULL)
> +        {
> +          const size_t __len = strlen(__old) + 1;
> +          __sav = new char[__len];
> +          memcpy(__sav, __old, __len);
> +          setlocale(LC_ALL, "C");
> +        }
>
>  #if !__LDBL_HAS_INFINITY__
>       errno = 0;
> diff --git a/libstdc++-v3/config/locale/generic/c_locale.h
b/libstdc++-v3/config/locale/generic/c_locale.h
> index 2c76000..6e6f673 100644
> --- a/libstdc++-v3/config/locale/generic/c_locale.h
> +++ b/libstdc++-v3/config/locale/generic/c_locale.h
> @@ -60,7 +60,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   {
>     char* __old = std::setlocale(LC_NUMERIC, 0);
>     char* __sav = 0;
> -    if (__builtin_strcmp(__old, "C"))
> +    if (__old != NULL && __builtin_strcmp(__old, "C"))
>       {
>        const size_t __len = __builtin_strlen(__old) + 1;
>        __sav = new char[__len];
> diff --git a/libstdc++-v3/config/locale/generic/time_members.cc
b/libstdc++-v3/config/locale/generic/time_members.cc
> index 3031075..fb9eb6e 100644
> --- a/libstdc++-v3/config/locale/generic/time_members.cc
> +++ b/libstdc++-v3/config/locale/generic/time_members.cc
> @@ -45,10 +45,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>           const tm* __tm) const throw()
>     {
>       char* __old = setlocale(LC_ALL, 0);
> -      const size_t __llen = strlen(__old) + 1;
> -      char* __sav = new char[__llen];
> -      memcpy(__sav, __old, __llen);
> -      setlocale(LC_ALL, _M_name_timepunct);
> +      char* __sav = NULL;
> +      if (__old != NULL)
> +        {
> +          const size_t __llen = strlen(__old) + 1;
> +          __sav = new char[__llen];
> +          memcpy(__sav, __old, __llen);
> +          setlocale(LC_ALL, _M_name_timepunct);
> +        }
>       const size_t __len = strftime(__s, __maxlen, __format, __tm);
>       setlocale(LC_ALL, __sav);
>       delete [] __sav;
> @@ -130,10 +134,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>           const tm* __tm) const throw()
>     {
>       char* __old = setlocale(LC_ALL, 0);
> -      const size_t __llen = strlen(__old) + 1;
> -      char* __sav = new char[__llen];
> -      memcpy(__sav, __old, __llen);
> -      setlocale(LC_ALL, _M_name_timepunct);
> +      char* __sav = NULL;
> +      if (__old != NULL)
> +        {
> +          const size_t __llen = strlen(__old) + 1;
> +          __sav = new char[__llen];
> +          memcpy(__sav, __old, __llen);
> +          setlocale(LC_ALL, _M_name_timepunct);
> +        }
>       const size_t __len = wcsftime(__s, __maxlen, __format, __tm);
>       setlocale(LC_ALL, __sav);
>       delete [] __sav;
>
>
> --
> This patch is available for review at http://codereview.appspot.com/4444046
>

[paolo.carlini_oracle.com, 2011-04-17 12:25:01]

Hi,
> Thanks Diego for committing this patch.
>
> Yes, I would like to submit the patch to trunk. The reason is for this
> patch is that setlocale() in bionicC always returns NULL (bionicC does
> not support setlocale()), however libstdc++ does not handle the NULL
> return value of setlocale(xxx,NULL) and thus causes segmentation
> fault.
The patch makes sense and I'm ok with it if you add a short comment 
explaining the issue, which we never encountered before. Of course, 
longer term we (you ;) may want to add to the library a 4th locale model 
beyond the existing ones, some sort of dummy locale model meant for 
targets like bionicC, which essentially don't provide locales at all, if 
I understand correctly: without a working setlocale how can you switch 
from one named locale to another in portable C code?

Paolo.