From patchwork Wed Dec 10 00:21:37 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 419344 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7FAF61400D2 for ; Wed, 10 Dec 2014 11:21:54 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; q=dns; s=default; b=Xa9QclBd5TV/PIKaDOAoPfDkxzOXSz 82+A9zfeY3CO1+JcWc9OaaFQ2SNazDVoBG4vVGEn92/hJhvOtmzE4UltJBE38HIc Z1P9UifXu7Sezpe6eRCeAar/qfedfLcM28kAkAInRmEbBEzjGNVvIzHJ6I66pMiB 94WqUW8VfFjAY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; s=default; bh=bBbTG6Ir+7fCn4nWdQ2k62lDK2Q=; b=iwkM fXYR9kZake0qU30r1AaXVMlIVaCsEWz6Y5TgE6IbHltOL573WjOmWmbLrDzuKRBQ BAwVfUkDbIOGvFBCHGgdz9scijDbPJz1PcHId5YCdmnuvw/eQw1i/3YSRXMWIklQ chwSbkOpAm7hoVevM6nnSK3jw6+InVTxX6p3E4k= Received: (qmail 10963 invoked by alias); 10 Dec 2014 00:21:44 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 10938 invoked by uid 89); 10 Dec 2014 00:21:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 10 Dec 2014 00:21:40 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBA0Lctk023973 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 9 Dec 2014 19:21:38 -0500 Received: from localhost (ovpn-116-46.ams2.redhat.com [10.36.116.46]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sBA0LbNZ008636; Tue, 9 Dec 2014 19:21:38 -0500 Date: Wed, 10 Dec 2014 00:21:37 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [patch] Some std::locale improvements Message-ID: <20141210002137.GH3134@redhat.com> References: <20141129225829.GS5191@redhat.com> <20141130204843.GU5191@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20141130204843.GU5191@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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 > __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 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 >(__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(_M_grouping[0]) > 0 - && (_M_grouping[0] + && static_cast(__grouping[0]) > 0 + && (__grouping[0] != __gnu_cxx::__numeric_traits::__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 >(__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(_M_grouping[0]) > 0 - && (_M_grouping[0] + && static_cast(__grouping[0]) > 0 + && (__grouping[0] != __gnu_cxx::__numeric_traits::__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 >(__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(...) {