race condition in boost::filesystem::path leads to crash when used in multithreaded programs

[aris.basic@…]

following app crashes on VC10 (Windows 7) (multibyte and Unicode builds) at point of creation of path (im working around the issue by doing dummy string to wstring conversion) [

wstring r; std::copy(s.begin(),s.end(),r.begin());

]

#include <boost/thread.hpp>
#include <boost/filesystem.hpp>


int main(void)
{
        std::string sPath("c:\\Development");
        
        boost::thread_group tg;

        for(int i = 0; i < 2; i++)
        {
                tg.create_thread([&sPath](){
                        boost::this_thread::sleep(boost::posix_time::milliseconds(10));
                        boost::filesystem::path p(sPath);

                        boost::filesystem::directory_iterator di(p), end;
                        while(di != end)
                                std::cout << (*(di++)).path().string() << std::endl;
                });
        }

        tg.join_all();

        int a;
        std::cin >> a;

}

VC10 CallStack:

>	msvcp100d.dll!std::codecvt<wchar_t,char,int>::in(int & _State, const char * _First1, const char * _Last1, const char * & _Mid1, wchar_t * _First2, wchar_t * _Last2, wchar_t * & _Mid2)  Line 1521 + 0x1f bytes	C++
 	filesystem_crash.exe!`anonymous namespace'::convert_aux(const char * from, const char * from_end, wchar_t * to, wchar_t * to_end, std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & target, const std::codecvt<wchar_t,char,int> & cvt)  Line 84 + 0x25 bytes	C++
 	filesystem_crash.exe!boost::filesystem3::path_traits::convert(const char * from, const char * from_end, std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & to, const std::codecvt<wchar_t,char,int> & cvt)  Line 165 + 0x20 bytes	C++
 	filesystem_crash.exe!boost::filesystem3::path_traits::dispatch<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & c, std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & to, const std::codecvt<wchar_t,char,int> & cvt)  Line 174 + 0x7e bytes	C++
 	filesystem_crash.exe!boost::filesystem3::path::path<std::basic_string<char,std::char_traits<char>,std::allocator<char> > >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & source, void * __formal)  Line 135 + 0x13 bytes	C++
 	filesystem_crash.exe!`anonymous namespace'::<lambda0>::operator()()  Line 15 + 0x10 bytes	C++
 	filesystem_crash.exe!boost::detail::thread_data<`anonymous namespace'::<lambda0> >::run()  Line 62	C++
 	filesystem_crash.exe!boost::`anonymous namespace'::thread_start_function(void * param)  Line 177	C++
 	msvcr100d.dll!_callthreadstartex()  Line 314 + 0xf bytes	C
 	msvcr100d.dll!_threadstartex(void * ptd)  Line 297	C

[wolfgang.fertsak@…]

I think the problem is in file filesystem/v3/source/path.cpp in method

const path::codecvt_type *& path::wchar_t_codecvt_facet() {

static const std::codecvt<wchar_t, char, std::mbstate_t> *

facet(

&std::use_facet<std::codecvt<wchar_t, char, std::mbstate_t> >

(path_locale()));

return facet;

}

This method is entered by multiple threads concurrently and the static pointer "facet" gets initialized by the first thread (which takes some time) but the other threads don't wait and just continue while facet is a NULL pointer.

In boost 1.44 this crash didn't occur. It seems that the initialization of global variable const fs::path dot_path(L"."); in filesystem/v3/source/path.cpp resulted in a call of path::wchar_t_codecvt_facet(), so the static "facet" pointer got initialized during program startup while no other threads were running.

In boost 1.48 the initialization of the same globale variable doesn't result in a call of path::wchar_t_codecvt_facet() so race conditions might occur during initialization of the static "facet" pointer (in C++ 03).

[Beman Dawes]

(In [76303]) Fix #4889, #6320, Locale codecvt_facet not thread safe on Windows. Move Windows, Mac OS X, locale and codecvt facet back to namespace scope. POSIX except OS X uses local static initialization (IE lazy) to ensure exceptions are catchable if environmental variables are misconfigured and to avoid use of locale("") if not actually used.

[anonymous]

This issue still appears to be present, using:

MSVC 2012 (Win7, 32-bit). Statically linked to boost::filesystem.

Just modified the example code to use "C:/" as the directory search path (to ensure existence).

Code asserts in .../libs/filesystem/src/path.cpp, line 888:

#if defined(BOOST_WINDOWS_API) && defined(BOOST_FILESYSTEM_STATIC_LINK)

  const path::codecvt_type& path::codecvt()
  {
    BOOST_ASSERT_MSG(codecvt_facet_ptr(), "codecvt_facet_ptr() facet hasn't been properly initialized");
    return *codecvt_facet_ptr();
  }

[jacob.schloss@…]

I agree the issue is still present, I am getting crashes due to invalid access inside the path conversion routines. As original poster says issue is that the initialization of static members is not threadsafe in all pre c++11 compilers. This seems to be compiler dependent, gcc seems to default to adding serialization code, but provides -fno-threadsafe-statics to disable, ​MSVC seems to not be. I am currently using MSVC 2010.

Although this comment states that the local is thread local

//  The path locale, which is global to the thread, can be changed by the
//  imbue() function. It is initialized to an implementation defined locale.

It looks like the same path object is shared between threads

  std::locale& path_locale()
  {
    static std::locale loc(default_locale());
    return loc;
  }

Adding explicit one time initialization code to the shared local object with boost::call_once works for me.

  void make_loc(std::locale& loc)
  {
	  loc = default_locale();
  }

  std::locale& path_locale()
  {
    static std::locale loc;

    static boost::once_flag once;
    boost::call_once(once, boost::bind(&make_loc, boost::ref(loc)));
    
    return loc;
  }

  void make_wchar_t_codecvt_facet(const path::codecvt_type *& cvt)
  {
	  cvt = &std::use_facet<std::codecvt<wchar_t, char, std::mbstate_t> >
		  (path_locale());
  }

  const path::codecvt_type *& path::wchar_t_codecvt_facet()
  {
   static const std::codecvt<wchar_t, char, std::mbstate_t> *
     facet;

   static boost::once_flag once;
   boost::call_once(once, boost::bind(&make_wchar_t_codecvt_facet, boost::ref(facet)));

   return facet;
  }

Another workaround that seemed to work is to do a path string operation on one thread in advance of multithread operations.

[jacob.schloss@…]

The call once struct should probably also not be static, I should pull those out.

[jacob.schloss@…]

Turning on /we4640 to make these warnings errors produces a large number of hits, there are other places where const static "empty path" objects are local to functions.

[Beman Dawes]

Boost.Filesystem's implementation of path locale and codecvt handling has been rewritten with much simpler, more portable, and hopefully more robust code. The interface has not changed. The rewrite has been applied to svn trunk as of revision 83062. An added reference documentation section (boost-root/libs/filesystem/doc/reference.html#path-Usage) describes class path usage concerns, such as thread data races.

These changes should resolve this issue. If you believe it has not been resolved satisfactorily, please open a new issue rather than reopening this issue. But before you do that, please read boost-root/libs/filesystem/doc/reference.html#path-Usage to be sure that the problem you are seeing is not a manifestation of one of the those concerns. If you do open a new issue, please be specific and supply a test case and lots of details. Just saying something "doesn't work" or "crashes" is not enough!

Thanks,

--Beman

[alexey.meteorite@…]

I used revision 83062 and it still crashed because of race condition in codecvt(). Call to codecvt() resulted from my call to fs::exists. I used VC9 and static linking.
I uncommented locale_mutex and its locks in codecvt() and imbue(const std::locale& loc), now everything is fine.

So, please, include mutex into next boost release. Or at least provide some build variant with it, that can be turned on.

[Vadim Panin]

Have not found "boost-root/libs/filesystem/doc/reference.html#path-Usage" section you are referring to. The following workaround works for me:

#ifdef _MSC_VER
		// fix for https://svn.boost.org/trac/boost/ticket/6320
		boost::filesystem::path::imbue( std::locale( "" ) );
#endif

if placed in main().

[anonymous]

If you're using Boost as a static lib in a DLL you have to apply the fix in the DLLMain example:

#ifdef WIN32
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif
#include <Windows.h>
#include <boost/filesystem/path.hpp>

static void init_boost () {
  boost::filesystem::path p("dummy");
};

BOOL APIENTRY DllMain( HMODULE /* hModule */,
                       DWORD  ul_reason_for_call,
                       LPVOID /* lpReserved */)
{
  switch (ul_reason_for_call)
  {
    case DLL_PROCESS_ATTACH:
      init_boost();
      break;
    case DLL_THREAD_ATTACH:
    case DLL_THREAD_DETACH:
    case DLL_PROCESS_DETACH:
    break;
  }
  return TRUE;
}
#endif

[dwieselwind@…]

I'm experiencing this too on 1.55 and MSVC10. Vadim Panin's hack works fine for me.

[anonymous]

Do not something in DllMain.

When called DLL_PROCESS_ATTACH/DLL_PROCESS_DETACH system is inoperative.

[anonymous]

Experiencing this on 1.61.0 too.

[anonymous]

1.61.0 MSVC120

[anonymous]

I exeperience the same issue with 1.60.0 with VS2010

[stl]

Just had 1.61.0 crash due to this issue and investigated a bit. The root of the problem is the initialization of a local static inside path_locale().

See path.cpp line 925:

static std::locale loc(default_locale());

This obviously leads to a race between threads with compilers that do not implement thread-safe initialization of local statics. BTW, none of the MSVC compilers before VC2015 do implement this feature, which Microsoft likes to call "magic statics".

So I did some tests with Visual Studio 2015 Update 3. I tested with the default v140 toolset and with the XP-compatibility toolset v140_xp. Here are the rather interesting results:

v140 toolset: works fine. v140_xp toolset: race, crash and burn.

If you step through the code compiled with v140, you can see the thread sync stubs that are automatically inserted by v140. Step through the same code compiled with v140_xp -> no thread sync stubs.

[stl]

Replying to stl:

v140 toolset: works fine.
v140_xp toolset: race, crash and burn.

Clarification: the above mentioned toolsets can be selected in VS 2015 C++ project settings, and are not to be confused with the bjam "toolset" parameter. In the above tests I used a special build of boost built with the VS2015 v140_xp toolset. You cannot normally do this with unmodified boost.build scripts.

However, Windows XP woes aside, the boost download page clearly states for the Visual C++ compilers that the following compilers are supported:

Visual C++: 7.1, 8.0, 9.0, 10.0, 11.0, 12.0, 14.0

This bug is still present in the latest boost v1.62.0 release, and leads to crashes with all of the above compilers up to 12.0, and even with 14.0 when using the command line switch "/Zc:threadSafeInit-" which disables thread-safe initialization of scoped statics [and is the default with v140_xp toolset for DLLs using ATL].

[stl]

Some simplified code to reproduce the crash. As concurrency bugs are timing dependent, this will crash most of the time, but not always.

#include <boost/filesystem.hpp>
#include <boost/thread.hpp>

void thr()
	{
	boost::filesystem::path file(L"C:\\test");
	boost::this_thread::sleep(boost::posix_time::milliseconds(10));
	std::cout << file.string() << std::endl;
	}


int main()
	{
	boost::thread_group tg;
	for (size_t n = 0; n < 5; ++n)
		{
		tg.create_thread(thr);
		}
	tg.join_all();
	return 0;
	}

[stl]

Replying to stl:

Some simplified code to reproduce the crash. As concurrency bugs are timing dependent, this will crash most of the time, but not always.

#include <boost/filesystem.hpp>
#include <boost/thread.hpp>

void thr()
	{
	boost::filesystem::path file(L"C:\\test");
	boost::this_thread::sleep(boost::posix_time::milliseconds(10));
	std::cout << file.string() << std::endl;
	}


int main()
	{
	boost::thread_group tg;
	for (size_t n = 0; n < 5; ++n)
		{
		tg.create_thread(thr);
		}
	tg.join_all();
	return 0;
	}

Of course, this code is meant to reproduce the crash on Windows, where boost::filesystem::path uses wide characters natively, and thus must convert when using string().

[valentin.kotolup@…]

BOOST 1.61.0. MSVC 12 Update 4

std::string some_path("C:\\something");
boost::filesystem::file_size(some_path);

Workaround from Vadim Panin works.

[anonymous]

Replying to stl:

Replying to stl:

Some simplified code to reproduce the crash. As concurrency bugs are timing dependent, this will crash most of the time, but not always.

#include <boost/filesystem.hpp>
#include <boost/thread.hpp>

void thr()
	{
	boost::filesystem::path file(L"C:\\test");
	boost::this_thread::sleep(boost::posix_time::milliseconds(10));
	std::cout << file.string() << std::endl;
	}


int main()
	{
	boost::thread_group tg;
	for (size_t n = 0; n < 5; ++n)
		{
		tg.create_thread(thr);
		}
	tg.join_all();
	return 0;
	}

Of course, this code is meant to reproduce the crash on Windows, where boost::filesystem::path uses wide characters natively, and thus must convert when using string().

boost::barrier can help a lot to catch threading issues:

const int num = 10;
boost::barrier b(num + 1);
std::vector<std::thread> threads;
for (int i = 0; i < num; ++i)
	threads.emplace_back([&b] { b.wait(); boost::filesystem::path::codecvt(); });

b.wait();

for (auto& thread: threads)
	thread.join();

[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.

[anonymous]

Just noticed that one load could be spared:

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

[anonymous]

With 1.64 it is still there. Has it some priority? Or it will be gone with static inicializing thread-safe compilers (I use MSVC 2013, which is probably not)?

Unhandled exception thrown: read access violation.
_Loc._Ptr was nullptr.

Incriminated thread (optimized code):

	[Inline Frame] My.dll!std::locale::_Getfacet(unsigned __int64) Line 468
 	My.dll!std::use_facet<std::codecvt<wchar_t,char,int> >(const std::locale & _Loc) Line 572
 	My.dll!MyCode() Line 692 // calling boost::filesystem::path(const std::string &)

I also suspect path_locale(), where is static inicializing global shared object across threads.

[sali98@…]

Kind of curious, why this one is not fixed until now? We have found a few crash reports related to this bug.

Maybe we have to upgrade from vs2013 to vs2015? Unfortunately, our code base is not ready for that yet:-(

[eksea@…]

Replying to 匿名用户:

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.

This workaround works for me.

[anonymous]

I am working with 1.66 on windows and experiencing the issue. Has there been focus on this to address it?

[alexandre.nunes@…]

I'm hit by this too.

[anonymous]

I've also encountered this issue.