1) locale::global(const locale&) is defined so: locale locale::global(const locale& __other) { _S_initialize(); // XXX MT _Impl* __old = _S_global; __other._M_impl->_M_add_reference(); _S_global = __other._M_impl; if (_S_global->_M_check_same_name() && (std::strcmp(_S_global->_M_names[0], "*") != 0)) setlocale(LC_ALL, __other.name().c_str()); // Reference count sanity check: one reference removed for the // subsition of __other locale, one added by return-by-value. Net // difference: zero. When the returned locale object's destrutor // is called, then the reference count is decremented and possibly // destroyed. return locale(__old); } If two threads run locale::global() at the same time, such that: 1) thread 1 begins and executes _Impl* __old = _S_global; and then yields. 2) thread 2 runs locale::global() completely. 3) thread 1 resumes executions and completes. Then the reference count of __old is decremented twice, although _S_global only holds 1 reference. To compensate, the reference count of the locale assigned to _S_global in thread 2 is incremented, but no reference to it is stored so is leaked. 2) locale::locale is defined so: locale::locale() throw() { _S_initialize(); (_M_impl = _S_global)->_M_add_reference(); } If 1) thread 1 runs locale::locale() and yields after executing _M_impl = _S_global but before calling _M_add_reference(), and the reference count of _S_global is 1. 2) thread 2 runs locale::global() completely and decrements the reference count of _S_global. 3) thread 1 resumes execution and calls _M_impl->_M_add_reference() The last reference to _S_global is removed in 2), so _M_add_reference() in 3) is called on an object that has already been deleted.
Confirmed (should be using the atomic versions or a lock).
Trying to fix it...
Created attachment 5231 [details] Perhaps...
Hi Pétur, what about the attached? I'm not a MT expert, but naively appears to answer your concerns... Paolo.
... forget about the locale::global part: I have to think a little more about it. Paolo.
To me a MT person but not really knowing the code that well, this looks better.
I did apply the tentative patch 12658 to the current 3.4 snapshot and tested on a i386 SMP machine. It does not solve the problem. I doubt in general that any reference counting approach ever gets thread safe. H.
Well......... something like this will work. It's modeled off of the glibc setlocale bits, and I think it will work. Thoughts? 2003-12-13 Benjamin Kosnik <bkoz@redhat.com> PR libstdc++/12658 * include/Makefile.am (bits_headers): Add concurrence.h. * include/Makefile.in: Regenerated. * include/bits/concurrence.h: New. * src/locale_init.cc: Use it. (locale::locale): Lock critical regions. (locale::global): Same. Index: include/Makefile.am =================================================================== RCS file: /cvs/gcc/gcc/libstdc++-v3/include/Makefile.am,v retrieving revision 1.72 diff -c -p -r1.72 Makefile.am *** include/Makefile.am 11 Nov 2003 20:09:07 -0000 1.72 --- include/Makefile.am 13 Dec 2003 06:32:27 -0000 *************** bits_headers = \ *** 101,106 **** --- 101,107 ---- ${bits_srcdir}/char_traits.h \ ${bits_srcdir}/codecvt.h \ ${bits_srcdir}/concept_check.h \ + ${bits_srcdir}/concurrence.h \ ${bits_srcdir}/cpp_type_traits.h \ ${bits_srcdir}/demangle.h \ ${bits_srcdir}/deque.tcc \ Index: include/bits/concurrence.h =================================================================== RCS file: include/bits/concurrence.h diff -N include/bits/concurrence.h *** /dev/null 1 Jan 1970 00:00:00 -0000 --- include/bits/concurrence.h 13 Dec 2003 06:32:27 -0000 *************** *** 0 **** --- 1,54 ---- + // Support for concurrent programing -*- C++ -*- + + // Copyright (C) 2003 + // Free Software Foundation, Inc. + // + // This file is part of the GNU ISO C++ Library. This library is free + // software; you can redistribute it and/or modify it under the + // terms of the GNU General Public License as published by the + // Free Software Foundation; either version 2, or (at your option) + // any later version. + + // This library is distributed in the hope that it will be useful, + // but WITHOUT ANY WARRANTY; without even the implied warranty of + // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + // GNU General Public License for more details. + + // You should have received a copy of the GNU General Public License along + // with this library; see the file COPYING. If not, write to the Free + // Software Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307, + // USA. + + // As a special exception, you may use this file as part of a free software + // library without restriction. Specifically, if other files instantiate + // templates or use macros or inline functions from this file, or you compile + // this file and link it with other files to produce an executable, this + // file does not by itself cause the resulting executable to be covered by + // the GNU General Public License. This exception does not however + // invalidate any other reasons why the executable file might be covered by + // the GNU General Public License. + + #ifndef _CONCURRENCE + #define _CONCURRENCE 1 + + // GCC's thread abstraction layer + #include "bits/gthr.h" + + #if __GTHREADS + # ifdef __GTHREAD_MUTEX_INIT + # define __glibcxx_mutex_define_initialized(NAME) \ + __gthread_mutex_t NAME = __GTHREAD_MUTEX_INIT + # else + # define __glibcxx_mutex_define_initialized(NAME) \ + __gthread_mutex_t NAME; \ + __GTHREAD_MUTEX_INIT_FUNCTION(&NAME) + # endif + # define __glibcxx_mutex_lock(LOCK) __gthread_mutex_lock(&LOCK) + # define __glibcxx_mutex_unlock(LOCK) __gthread_mutex_unlock(&LOCK) + #else + # define __glibcxx_mutex_define_initialized(NAME) + # define __glibcxx_mutex_lock(LOCK) + # define __glibcxx_mutex_unlock(LOCK) + #endif + + #endif Index: src/locale_init.cc =================================================================== RCS file: /cvs/gcc/gcc/libstdc++-v3/src/locale_init.cc,v retrieving revision 1.3 diff -c -p -r1.3 locale_init.cc *** src/locale_init.cc 24 Nov 2003 17:24:40 -0000 1.3 --- src/locale_init.cc 13 Dec 2003 06:32:27 -0000 *************** *** 33,38 **** --- 33,39 ---- #include <cwctype> // For towupper, etc. #include <locale> #include <bits/atomicity.h> + #include <bits/concurrence.h> namespace __gnu_cxx { *************** namespace std *** 96,116 **** locale::locale() throw() { _S_initialize(); ! (_M_impl = _S_global)->_M_add_reference(); } locale locale::global(const locale& __other) { _S_initialize(); ! ! // XXX MT _Impl* __old = _S_global; __other._M_impl->_M_add_reference(); _S_global = __other._M_impl; if (_S_global->_M_check_same_name() && (std::strcmp(_S_global->_M_names[0], "*") != 0)) setlocale(LC_ALL, __other.name().c_str()); // Reference count sanity check: one reference removed for the // subsition of __other locale, one added by return-by-value. Net --- 97,121 ---- locale::locale() throw() { _S_initialize(); ! __glibcxx_mutex_define_initialized(lock); ! __glibcxx_mutex_lock(lock); ! (_M_impl = _S_global)->_M_add_reference(); ! __glibcxx_mutex_unlock(lock); } locale locale::global(const locale& __other) { _S_initialize(); ! __glibcxx_mutex_define_initialized(lock); ! __glibcxx_mutex_lock(lock); _Impl* __old = _S_global; __other._M_impl->_M_add_reference(); _S_global = __other._M_impl; if (_S_global->_M_check_same_name() && (std::strcmp(_S_global->_M_names[0], "*") != 0)) setlocale(LC_ALL, __other.name().c_str()); + __glibcxx_mutex_unlock(lock); // Reference count sanity check: one reference removed for the // subsition of __other locale, one added by return-by-value. Net
PS there needs to be/should be a threads testcase for this. any takers?
Subject: Re: Thread safety problems in locale::global() and locale::locale() >------- Additional Comments From bkoz at gcc dot gnu dot org 2003-12-13 >Well......... something like this will work. It's modeled off of the glibc >setlocale bits, and I think it will work. >Thoughts? >2003-12-13 Benjamin Kosnik <bkoz@redhat.com> > > PR libstdc++/12658 > * include/Makefile.am (bits_headers): Add concurrence.h. > * include/Makefile.in: Regenerated. > * include/bits/concurrence.h: New. > * src/locale_init.cc: Use it. > (locale::locale): Lock critical regions. > (locale::global): Same. It seems odd to wrap atomic updates with a mutex but it looks OK... I must commend the improvement of adding bits/concurrence.h instead of continuing the ad hoc style which is scattered about. - Loren
Subject: Re: Thread safety problems in locale::global() and locale::locale() bkoz at gcc dot gnu dot org wrote: >------- Additional Comments From bkoz at gcc dot gnu dot org 2003-12-13 06:35 ------- > >Well......... something like this will work. It's modeled off of the glibc >setlocale bits, and I think it will work. > >Thoughts? > > Probably, for locale::global() a lock is really needed, I agree with you. However, for this > locale::locale() throw() > { > _S_initialize(); >! (_M_impl = _S_global)->_M_add_reference(); > } > > Are you really sure that the trivial rearrangement that I proposed doesn't suffice? It appears to answer Pétur's analysis... Paolo.
Subject: Re: Thread safety problems in locale::global() and locale::locale() >It seems odd to wrap atomic updates with a mutex but it looks OK... Yeah, for locale::locale()? For locale::global it looks less strange, I think. Sadly, I think it is necessary for locale::locale(), as assignment + atomic update is not, in combination, atomic... I suppose I could split that into two lines to clarify. >I must commend the improvement of adding bits/concurrence.h instead of >continuing the ad hoc style which is scattered about. - Loren Well......... \o.o/ You know my feelings about stl_thread.h.... I was just testing the water here, to see your reaction. If this is ok then I think I'm going to go through and consolidate, and try to get rid of the _GTHREADS bits that have been multiplying in the codebase. Thus, the placement of bits/concurrence.h as a public header. That would mean adding similar bits for _gthread_once, usage of which has multiplied as of late, but it seems very doable. That's a later patch, however... Thanks, benjamin
Paolo, my bad. I'm sorry. You are correct. I think just splitting locale::locale() up into: + _S_initialize(); + _S_global->_M_add_reference(); + _M_impl = _S_global; Will be sufficient, without the heavyweight locks, which is good. Loren, isn't this correct? This would solve your comment as well. -benjamin
>+ _S_initialize(); >+ _S_global->_M_add_reference(); >+ _M_impl = _S_global; This still looks wrong. Consider if one thread runs locale::locale() and yields after _S_global->_M_add_reference(); Another thread then runs locale::global() from start to finish. The second thread then completes locale::locale(). In that case locale::locale() increases the reference count of the old global locale, but keeps a pointer to the new global locale. The old global locale gets leaked, and the new global locale will be deleted while there are still pointers to it.
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Changes by: bkoz@gcc.gnu.org 2003-12-15 21:08:03 Modified files: libstdc++-v3 : ChangeLog libstdc++-v3/include: Makefile.am Makefile.in libstdc++-v3/src: locale_init.cc Added files: libstdc++-v3/include/bits: concurrence.h Log message: 2003-12-15 Benjamin Kosnik <bkoz@redhat.com> PR libstdc++/12658 * include/Makefile.am (bits_headers): Add concurrence.h. * include/Makefile.in: Regenerated. * include/bits/concurrence.h: New. * src/locale_init.cc: Use it. (locale::locale): Lock critical regions. (locale::global): Same. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2158&r2=1.2159 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/Makefile.am.diff?cvsroot=gcc&r1=1.72&r2=1.73 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/Makefile.in.diff?cvsroot=gcc&r1=1.92&r2=1.93 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/concurrence.h.diff?cvsroot=gcc&r1=NONE&r2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/locale_init.cc.diff?cvsroot=gcc&r1=1.4&r2=1.5
Actually took care of it.
Fixed in 3.4.
There is a problem with the patch: http://gcc.gnu.org/ml/libstdc++/2004-01/msg00051.html
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Changes by: ljrittle@gcc.gnu.org 2004-01-07 17:40:46 Modified files: libstdc++-v3 : ChangeLog libstdc++-v3/src: locale_init.cc Log message: (re-open) PR libstdc++/12658 * src/locale_init.cc (locale::locale): Remove ill-scoped mutex. (locale::global): Likewise. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2200&r2=1.2201 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/locale_init.cc.diff?cvsroot=gcc&r1=1.5&r2=1.6
Created attachment 5430 [details] Test case This fails on gcc 3.3.2 and 3.2.3 (as expected). This also fails with the gcc-3.4-20031231 snapshot. This test case is non-deterministic, so the time it runs before it fails can vary a lot (and sometimes it will succeed). My setup: 500MHz Pentium III with 512MB ram Red Hat Linux 9 Kernel 2.4.20-8 glibc-2.3.2-27.9
Confirmed. Well, back to the drawing board with this one.
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Changes by: bkoz@gcc.gnu.org 2004-03-07 01:32:43 Modified files: libstdc++-v3 : ChangeLog libstdc++-v3/config: linker-map.gnu libstdc++-v3/config/cpu/generic: atomicity.h libstdc++-v3/config/cpu/hppa: atomicity.h libstdc++-v3/config/os/hpux: os_defines.h libstdc++-v3/include/bits: concurrence.h libstdc++-v3/include/ext: mt_allocator.h libstdc++-v3/src: locale_init.cc misc-inst.cc Log message: 2004-03-06 Benjamin Kosnik <bkoz@redhat.com> PR libstdc++/12658 * src/locale_init.cc (locale::locale): Lock critical regions with external mutexes. (locale::global): Same. * include/bits/concurrence.h (__glibcxx_mutex_define_initialized): Add in once bits for cases without __GTHREAD_MUTEX_INIT. (__glibcxx_mutex_lock): Same. * config/cpu/generic/atomicity.h: Remove _GLIBCXX_NEED_GENERIC_MUTEX, use concurrence.h. * src/misc-inst.cc: Move all locking bits out of this file. * config/os/hpux/os_defines.h: Remove _GLIBCXX_INST_ATOMICITY_LOCK. * src/misc-inst.cc: Same. * config/cpu/hppa/atomicity.h: Same. * config/linker-map.gnu: Remove types in the signature of atomic exports, as they may vary. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2383&r2=1.2384 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/linker-map.gnu.diff?cvsroot=gcc&r1=1.57&r2=1.58 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/cpu/generic/atomicity.h.diff?cvsroot=gcc&r1=1.8&r2=1.9 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/cpu/hppa/atomicity.h.diff?cvsroot=gcc&r1=1.8&r2=1.9 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/os/hpux/os_defines.h.diff?cvsroot=gcc&r1=1.10&r2=1.11 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/concurrence.h.diff?cvsroot=gcc&r1=1.2&r2=1.3 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/ext/mt_allocator.h.diff?cvsroot=gcc&r1=1.17&r2=1.18 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/locale_init.cc.diff?cvsroot=gcc&r1=1.9&r2=1.10 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/misc-inst.cc.diff?cvsroot=gcc&r1=1.26&r2=1.27
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Changes by: bkoz@gcc.gnu.org 2004-03-08 22:11:48 Modified files: libstdc++-v3 : ChangeLog Added files: libstdc++-v3/testsuite/22_locale/locale/cons: 12658_thread.cc Log message: 2004-03-08 Petur Runolfsson <peturr02@ru.is> PR libstdc++/12658 * testsuite/22_locale/locale/cons/12658_thread.cc: New. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2388&r2=1.2389 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/22_locale/locale/cons/12658_thread.cc.diff?cvsroot=gcc&r1=NONE&r2=1.1
This fix is confirmed, and the new fix didn't kill BSD4. -benjamin
The problem in locale::locale() is still present: __glibcxx_mutex_define_initialized(locale_cons_mutex); __glibcxx_mutex_define_initialized(locale_global_mutex); } // namespace __gnu_internal namespace std { using namespace __gnu_internal; locale::locale() throw() : _M_impl(0) { _S_initialize(); __gnu_cxx::lock sentry(__gnu_internal::locale_cons_mutex); _S_global->_M_add_reference(); _M_impl = _S_global; } locale locale::global(const locale& __other) { _S_initialize(); _Impl* __old; { __gnu_cxx::lock sentry(__gnu_internal::locale_global_mutex); __old = _S_global; __other._M_impl->_M_add_reference(); _S_global = __other._M_impl; Note that while locale::locale() and locale::global() both lock a mutex before touching _S_global, they lock different mutexes. There is thus nothing to stop one thread from executing locale::global() while another executes locale::locale(). As already noted, this can (and does) lead to serious problems.
Created attachment 6846 [details] Test case Modified from original test case so that it now calls locale::locale().
Before anyone says "don't do that then" ... This sort of usage can be perfectly safe (and sometimes even neccessary): Thread 1 needs to do some formatting with some crufty C code, and thus needs to set the global locale: void thread1() { // Set global locale to "C" locale tmp = locale::global(locale("C")); // Do some C style formatting fprintf(f, ...); // Reset global locale locale::global(tmp); } Thread 2 also needs to format, but uses IOstreams. Because thread 1 may be playing games with the global locale, take care always to call imbue: void thread2() { // Thread specific locale locale myloc(""); fstream f; // Take care not to use global locale, can't be trusted. f.imbue(myloc); // Do some C++ style formatting f << ... } The two threads don't share any user-visible data, so this sort of usage should be thread-safe. The only problem is that fstream::fstream() will call locale::locale(), which touches _S_global, a variable that the user should need to know about.
Created attachment 6847 [details] Tentative patch for the issue now pointed out Thanks Pétur. Indeed, I concur with your new analysis. Therefore, what about using just *one* mutex like in the attached?? Paolo.
> what about using just *one* mutex like in the attached?? Yes, that should fix it.
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Changes by: paolo@gcc.gnu.org 2004-07-29 15:54:50 Modified files: libstdc++-v3 : ChangeLog libstdc++-v3/src: locale_init.cc Log message: 2004-07-29 Paolo Carlini <pcarlini@suse.de> Petur Runolfsson <peturr02@ru.is> PR libstdc++/12658 (continued) * src/locale_init.cc (locale::locale, locale::global): Use a single locale_mutex instead of two separate mutexes. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2590&r2=1.2591 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/locale_init.cc.diff?cvsroot=gcc&r1=1.14&r2=1.15
Subject: Bug 12658 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-3_4-branch Changes by: paolo@gcc.gnu.org 2004-07-30 07:52:32 Modified files: libstdc++-v3 : ChangeLog libstdc++-v3/src: locale_init.cc libstdc++-v3/testsuite/22_locale/locale/cons: 12658_thread.cc libstdc++-v3/testsuite/22_locale/num_put/put/char: 14220.cc libstdc++-v3/testsuite/22_locale/num_put/put/wchar_t: 14220.cc libstdc++-v3/docs/html/ext: lwg-active.html lwg-defects.html Log message: 2004-07-30 Paolo Carlini <pcarlini@suse.de> Petur Runolfsson <peturr02@ru.is> PR libstdc++/12658 (continued) * src/locale_init.cc (locale::locale, locale::global): Use a single locale_mutex instead of two separate mutexes. 2004-07-30 Paolo Carlini <pcarlini@suse.de> * testsuite/22_locale/locale/cons/12658_thread.cc: Xfail: due to a bug in glibcs older than 2004-07-16, it can unpredictably fail irrespective of the correctness of libstdc++. 2004-07-30 Benjamin Kosnik <bkoz@redhat.com> * src/locale_init.cc: Use __gnu_cxx::lock. 2004-07-30 Paolo Carlini <pcarlini@suse.de> * testsuite/22_locale/num_put/put/char/14220.cc: Increase the value of precision: 10 is too small to actually trigger the bug. * testsuite/22_locale/num_put/put/wchar_t/14220.c: Likewise. 2004-07-30 Paolo Carlini <pcarlini@suse.de> * docs/html/ext/lwg-active.html, lwg-defects.html: Import Revision 31. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.2224.2.149&r2=1.2224.2.150 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/locale_init.cc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.6.4.4&r2=1.6.4.5 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/22_locale/locale/cons/12658_thread.cc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.2.2.1&r2=1.2.2.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/22_locale/num_put/put/char/14220.cc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.1.2.1&r2=1.1.2.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/22_locale/num_put/put/wchar_t/14220.cc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.1.2.1&r2=1.1.2.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/docs/html/ext/lwg-active.html.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.6.4.1&r2=1.6.4.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/docs/html/ext/lwg-defects.html.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.6.4.1&r2=1.6.4.2
Fixed (again ;) everywhere.