Re: [patch] Some std::locale improvements

[Jonathan Wakely <jwakely at redhat dot com>]

On 30/11/14 20:48 +0000, Jonathan Wakely wrote:
> I think we also need this to make __numpunct_cache and
> __moneypunct_cache exception-safe. If we set _M_allocated=true at the
> start of the _M_cache functions and then an allocation throws we will
> delete[] the memory allocated in _M_cache, but then the cache's
> destructor will see _M_allocated==true and will try to delete[] them
> again:
>
> template<typename _CharT>
>   __numpunct_cache<_CharT>::~__numpunct_cache()
>   {
>     if (_M_allocated)
>       {
>         delete [] _M_grouping;
>         delete [] _M_truename;
>         delete [] _M_falsename;
>       }
>   }
>
> Delaying setting the bool and the pointers themselves until after all
> allocations avoids that.
>
> The _M_cache functions were also calling virtual functions twice when
> once is enough.
>
> Finally, __timepunct::_M_cache() is declared but never defined, so I
> want to remove that.
>
> Tested x86_64-linux and powerpc64-linux.
>
> Does anyone see anything wrong with my reasoning above before I commit
> this?

I've committed this to trunk. I was only able to reproduce the
double-free by hacking the library code to throw in that function, so
I've verified manually that this changes fixes it, but don't have
anything to put in the testsuite.

commit 1e79c0e5f5322fcc80a840b4eb3ce2f3b92133fd
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Dec 9 16:59:19 2014 +0000

    	* include/bits/locale_facets.tcc (numpunct::_M_cache): Avoid calling
    	virtual functions twice. Only update _M_allocated after all
    	allocations have succeeded.
    	* include/bits/locale_facets_nonio.tcc (moneypunct::_M_cache):
    	Likewise.
    	* include/bits/locale_facets_nonio.h (__timepunct::_M_cache): Remove
    	unused declaration.

diff --git a/libstdc++-v3/include/bits/locale_facets.tcc b/libstdc++-v3/include/bits/locale_facets.tcc
index 88adc0d..200e099 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -77,8 +77,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     __numpunct_cache<_CharT>::_M_cache(const locale& __loc)
     {
-      _M_allocated = true;
-
       const numpunct<_CharT>& __np = use_facet<numpunct<_CharT> >(__loc);
 
       char* __grouping = 0;
@@ -86,24 +84,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _CharT* __falsename = 0;
       __try
 	{
-	  _M_grouping_size = __np.grouping().size();
+	  const string& __g = __np.grouping();
+	  _M_grouping_size = __g.size();
 	  __grouping = new char[_M_grouping_size];
-	  __np.grouping().copy(__grouping, _M_grouping_size);
-	  _M_grouping = __grouping;
+	  __g.copy(__grouping, _M_grouping_size);
 	  _M_use_grouping = (_M_grouping_size
-			     && static_cast<signed char>(_M_grouping[0]) > 0
-			     && (_M_grouping[0]
+			     && static_cast<signed char>(__grouping[0]) > 0
+			     && (__grouping[0]
 				 != __gnu_cxx::__numeric_traits<char>::__max));
 
-	  _M_truename_size = __np.truename().size();
+	  const basic_string<_CharT>& __tn = __np.truename();
+	  _M_truename_size = __tn.size();
 	  __truename = new _CharT[_M_truename_size];
-	  __np.truename().copy(__truename, _M_truename_size);
-	  _M_truename = __truename;
+	  __tn.copy(__truename, _M_truename_size);
 
-	  _M_falsename_size = __np.falsename().size();
+	  const basic_string<_CharT>& __fn = __np.falsename();
+	  _M_falsename_size = __fn.size();
 	  __falsename = new _CharT[_M_falsename_size];
-	  __np.falsename().copy(__falsename, _M_falsename_size);
-	  _M_falsename = __falsename;
+	  __fn.copy(__falsename, _M_falsename_size);
 
 	  _M_decimal_point = __np.decimal_point();
 	  _M_thousands_sep = __np.thousands_sep();
@@ -115,6 +113,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  __ct.widen(__num_base::_S_atoms_in,
 		     __num_base::_S_atoms_in
 		     + __num_base::_S_iend, _M_atoms_in);
+
+	  _M_grouping = __grouping;
+	  _M_truename = __truename;
+	  _M_falsename = __falsename;
+	  _M_allocated = true;
 	}
       __catch(...)
 	{
diff --git a/libstdc++-v3/include/bits/locale_facets_nonio.h b/libstdc++-v3/include/bits/locale_facets_nonio.h
index 5c1eeb7..1b0aff9 100644
--- a/libstdc++-v3/include/bits/locale_facets_nonio.h
+++ b/libstdc++-v3/include/bits/locale_facets_nonio.h
@@ -138,9 +138,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ~__timepunct_cache();
 
-      void
-      _M_cache(const locale& __loc);
-
     private:
       __timepunct_cache&
       operator=(const __timepunct_cache&);
diff --git a/libstdc++-v3/include/bits/locale_facets_nonio.tcc b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
index c9f8dac..42c3504 100644
--- a/libstdc++-v3/include/bits/locale_facets_nonio.tcc
+++ b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
@@ -68,8 +68,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     __moneypunct_cache<_CharT, _Intl>::_M_cache(const locale& __loc)
     {
-      _M_allocated = true;
-
       const moneypunct<_CharT, _Intl>& __mp =
 	use_facet<moneypunct<_CharT, _Intl> >(__loc);
 
@@ -83,29 +81,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _CharT* __negative_sign = 0;     
       __try
 	{
-	  _M_grouping_size = __mp.grouping().size();
+	  const string& __g = __mp.grouping();
+	  _M_grouping_size = __g.size();
 	  __grouping = new char[_M_grouping_size];
-	  __mp.grouping().copy(__grouping, _M_grouping_size);
-	  _M_grouping = __grouping;
+	  __g.copy(__grouping, _M_grouping_size);
 	  _M_use_grouping = (_M_grouping_size
-			     && static_cast<signed char>(_M_grouping[0]) > 0
-			     && (_M_grouping[0]
+			     && static_cast<signed char>(__grouping[0]) > 0
+			     && (__grouping[0]
 				 != __gnu_cxx::__numeric_traits<char>::__max));
 
-	  _M_curr_symbol_size = __mp.curr_symbol().size();
+	  const basic_string<_CharT>& __cs = __mp.curr_symbol();
+	  _M_curr_symbol_size = __cs.size();
 	  __curr_symbol = new _CharT[_M_curr_symbol_size];
-	  __mp.curr_symbol().copy(__curr_symbol, _M_curr_symbol_size);
-	  _M_curr_symbol = __curr_symbol;
+	  __cs.copy(__curr_symbol, _M_curr_symbol_size);
 
-	  _M_positive_sign_size = __mp.positive_sign().size();
+	  const basic_string<_CharT>& __ps = __mp.positive_sign();
+	  _M_positive_sign_size = __ps.size();
 	  __positive_sign = new _CharT[_M_positive_sign_size];
-	  __mp.positive_sign().copy(__positive_sign, _M_positive_sign_size);
-	  _M_positive_sign = __positive_sign;
+	  __ps.copy(__positive_sign, _M_positive_sign_size);
 
-	  _M_negative_sign_size = __mp.negative_sign().size();
+	  const basic_string<_CharT>& __ns = __mp.negative_sign();
+	  _M_negative_sign_size = __ns.size();
 	  __negative_sign = new _CharT[_M_negative_sign_size];
-	  __mp.negative_sign().copy(__negative_sign, _M_negative_sign_size);
-	  _M_negative_sign = __negative_sign;
+	  __ns.copy(__negative_sign, _M_negative_sign_size);
 
 	  _M_pos_format = __mp.pos_format();
 	  _M_neg_format = __mp.neg_format();
@@ -113,6 +111,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  const ctype<_CharT>& __ct = use_facet<ctype<_CharT> >(__loc);
 	  __ct.widen(money_base::_S_atoms,
 		     money_base::_S_atoms + money_base::_S_end, _M_atoms);
+
+	  _M_grouping = __grouping;
+	  _M_curr_symbol = __curr_symbol;
+	  _M_positive_sign = __positive_sign;
+	  _M_negative_sign = __negative_sign;
+	  _M_allocated = true;
 	}
       __catch(...)
 	{