| Message ID | 20110416202056.81047AE18B@tobiano.tor.corp.google.com |
|---|---|
| State | New |
| Headers | show |
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 >
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.
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;
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. -- This patch is available for review at http://codereview.appspot.com/4444046