Re: [Boost-bugs] [Boost C++ Libraries] #6320: race condition in boost::filesystem::path leads to crash when used in multithreaded programs

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #6320: race condition in boost::filesystem::path leads to crash when used in multithreaded programs
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2017-05-09 16:07:28

#6320: race condition in boost::filesystem::path leads to crash when used in
multithreaded programs
-------------------------------+-------------------------
  Reporter:  aris.basic@…      |      Owner:  bemandawes
      Type:  Bugs              |     Status:  reopened
 Milestone:  To Be Determined  |  Component:  filesystem
   Version:  Boost 1.62.0      |   Severity:  Showstopper
Resolution:                    |   Keywords:
-------------------------------+-------------------------

Comment (by anonymous):

 I've got a suggestion to resolve the issue.

 I think we should rewrite {{{std::locale& path_locale()}}} in the unnamed
 namespace in path.cpp

 Instead of using {{{static std::locale}}} (the original code):
 {{{
 std::locale& path_locale()
 {
         static std::locale loc(default_locale());
         return loc;
 }
 }}}
 in pre-c++11 we could use an atomic pointer:
 {{{
 std::mutex& locale_mutex()
 {
   static std::mutex mutex;
   return mutex;
 }

 std::unique_ptr<std::locale>& locale_unique_ptr()
 {
   static std::unique_ptr<std::locale> locale;
   return locale;
 }

 std::atomic<std::locale*>& locale_pointer()
 {
   static std::atomic<std::locale*> locale;
   return locale;
 }

 std::locale& path_locale()
 {
   if (!locale_pointer().load())
   {
     std::lock_guard<std::mutex> l(locale_mutex());
     if (!locale_pointer().load())
     {
       locale_unique_ptr() =
 std::make_unique<std::locale>(default_locale());
       locale_pointer().store(locale_unique_ptr().get());
     }
   }
   return *locale_pointer().load();
 }
 }}}

 In the path.hpp we could add a dummy static object (int) to initialise the
 mutex, the unique_ptr and the atomic ptr.

 {{{
 namespace
 {
         int dummy = boost::filesystem::path::codecvt_init();
 }
 }}}

 This static dummy goes to every compilation unit where boost/path.hpp is
 included and causes the initilisation of the necessary local static helper
 objects (mutex, atomic and unique ptrs).

 {{{codecvt_init()}}} is a static member of the path class and it is
 defined in the end of the path.cpp file (where path::codecvt is defined,
 too):
 {{{
 int boost::filesystem::path::codecvt_init()
 {
         locale_mutex();
         locale_pointer();
         locale_unique_ptr();
         return 0;
 }
 }}}

 If our program never calls {{{path::codecvt}}} the unique ptr remains
 empty (but initialised!) so it is up the the client when the locale (and
 the facet) initialisation happens. This is important because on POSIX
 platforms it can result an exception to be thrown so we do not want to do
 the initialisation until the {{{main}}} is called. (Please note this is
 not always possible, e.g. there is a static logger class instance that
 uses {{{path::codecvt}}} before the {{{main}}} is reached.)

 Only bit I do not like (have not been able to sort out) that the path
 class public interface would have a new static function
 ({{{codecvt_init}}}). Calling it by the clients would not cause any harm,
 but it would be better to hide it somehow. I do not think this the end of
 the world. Eliminating the crash, i guess, is more important than paying a
 little price in the form of polluting the interface with a dummy function.

 I used {{{std::atomic}}}, {{{std::uinique_ptr}}}, {{{std::mutex}}} and
 {{{std::lock_guard}}} that have to be replaced with the corresponding
 boost classes. It is also neccessary to add an #if <pre c++11> ... #else
 ... #endif preprocessor condition to switch between the proposed or the
 original code.

 I wonder whether this would be less efficient than the local static stuff.
 1. Not if we use c++11 (the preprocessor condition would switch)
 2. I guess, even the local static magic implementation needs some kind of
 synchronisation for initialisation/access local static objects that may
 not be less costly than that the atomic ptr version demands

 I would be grateful if someone could comment on the idea. If you think
 it's worth considering it as a solution I could work on patch for review.

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/6320#comment:27>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.