Boost C++ Libraries: Ticket #6320: race condition in boost::filesystem::path leads to crash when used in multithreaded programs

wolfgang.fertsak@… — Tue, 27 Dec 2011 16:05:14 GMT

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

status changed; resolution set

Beman Dawes — Wed, 04 Jan 2012 14:53:51 GMT

status new → closed
resolution → fixed

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

status, version, severity changed; resolution deleted

anonymous — Wed, 31 Oct 2012 09:01:52 GMT

status closed → reopened
version Boost 1.48.0 → Boost 1.51.0
resolution fixed
severity Showstopper → Problem

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@… — Fri, 14 Dec 2012 18:38:15 GMT

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@… — Sat, 22 Dec 2012 00:31:59 GMT

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

jacob.schloss@… — Sat, 22 Dec 2012 00:33:32 GMT

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.

status changed; resolution set

Beman Dawes — Fri, 22 Feb 2013 13:35:41 GMT

status reopened → closed
resolution → fixed

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@… — Fri, 17 May 2013 13:03:27 GMT

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 — Fri, 23 Aug 2013 15:10:44 GMT

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 — Fri, 11 Oct 2013 11:52:08 GMT

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@… — Wed, 11 Jun 2014 05:31:32 GMT

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

status changed; resolution deleted

anonymous — Wed, 03 Dec 2014 12:26:35 GMT

status closed → reopened
resolution fixed

version changed

anonymous — Wed, 03 Dec 2014 12:28:14 GMT

version Boost 1.51.0 → Boost 1.52.0

anonymous — Tue, 31 Mar 2015 12:49:05 GMT

Do not something in DllMain.

When called DLL_PROCESS_ATTACH/DLL_PROCESS_DETACH system is inoperative.

version changed

anonymous — Sun, 20 Mar 2016 22:50:10 GMT

version Boost 1.52.0 → Boost 1.60.0

cc set

raad@… — Mon, 27 Jun 2016 13:08:35 GMT

cc raad@… added

anonymous — Sat, 17 Sep 2016 00:13:08 GMT

Experiencing this on 1.61.0 too.

anonymous — Sat, 17 Sep 2016 04:18:07 GMT

1.61.0 MSVC120

anonymous — Fri, 30 Sep 2016 12:43:13 GMT

I exeperience the same issue with 1.60.0 with VS2010

stl — Mon, 07 Nov 2016 16:57:26 GMT

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.

version changed

stl — Tue, 08 Nov 2016 17:40:03 GMT

version Boost 1.60.0 → Boost 1.62.0

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

severity, summary changed

stl — Tue, 08 Nov 2016 17:43:11 GMT

severity Problem → Showstopper
summary creation of path from std::string on Windows (VC10) crashes (access error) → race condition in boost::filesystem::path leads to crash when used in multithreaded programs

stl — Tue, 08 Nov 2016 17:56:23 GMT

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 — Tue, 08 Nov 2016 18:09:08 GMT

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@… — Thu, 17 Nov 2016 20:21:43 GMT

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 — Tue, 09 May 2017 15:07:24 GMT

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 — Tue, 09 May 2017 16:07:28 GMT

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.

Not if we use c++11 (the preprocessor condition would switch)
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 — Tue, 09 May 2017 16:28:48 GMT

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;
}

cc changed

daniel.kruegler@… — Tue, 09 May 2017 18:56:03 GMT

cc daniel.kruegler@… added

version changed

anonymous — Tue, 08 Aug 2017 10:37:30 GMT

version Boost 1.62.0 → Boost 1.64.0

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@… — Fri, 11 Aug 2017 21:36:33 GMT

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@… — Tue, 21 Nov 2017 03:07:22 GMT

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.

Not if we use c++11 (the preprocessor condition would switch)
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 — Fri, 23 Feb 2018 02:16:14 GMT

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

alexandre.nunes@… — Tue, 20 Mar 2018 00:10:50 GMT

I'm hit by this too.

anonymous — Sun, 22 Apr 2018 11:42:14 GMT

I've also encountered this issue.