Modify

Ticket #6320 (reopened Bugs)

Opened 5 years ago

Last modified 3 weeks ago

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

Reported by: aris.basic@… Owned by: bemandawes
Milestone: To Be Determined Component: filesystem
Version: Boost 1.62.0 Severity: Showstopper
Keywords: Cc: raad@…

Description

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

Attachments

Change History

comment:1 Changed 5 years ago by 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).

comment:2 Changed 5 years ago by bemandawes

  • Status changed from new to closed
  • Resolution set to 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.

comment:3 Changed 4 years ago by anonymous

  • Status changed from closed to reopened
  • Version changed from Boost 1.48.0 to Boost 1.51.0
  • Resolution fixed deleted
  • Severity changed from Showstopper to 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();
  }

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by jacob.schloss@…

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

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by bemandawes

  • Status changed from reopened to closed
  • Resolution set to 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

comment:8 Changed 4 years ago by 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.

comment:9 Changed 3 years ago by 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().

comment:10 Changed 3 years ago by 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

comment:11 Changed 2 years ago by dwieselwind@…

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

comment:12 Changed 2 years ago by anonymous

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:13 Changed 2 years ago by anonymous

  • Version changed from Boost 1.51.0 to Boost 1.52.0

comment:14 Changed 21 months ago by anonymous

Do not something in DllMain?.

When called DLL_PROCESS_ATTACH/DLL_PROCESS_DETACH system is inoperative.

comment:15 Changed 9 months ago by anonymous

  • Version changed from Boost 1.52.0 to Boost 1.60.0

comment:16 Changed 5 months ago by raad@…

  • Cc raad@… added

comment:17 Changed 3 months ago by anonymous

Experiencing this on 1.61.0 too.

comment:18 Changed 3 months ago by anonymous

1.61.0 MSVC120

comment:19 Changed 2 months ago by anonymous

I exeperience the same issue with 1.60.0 with VS2010

comment:20 follow-up: ↓ 21 Changed 4 weeks ago by 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.

comment:21 in reply to: ↑ 20 Changed 4 weeks ago by stl

  • Version changed from Boost 1.60.0 to 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].

comment:22 Changed 4 weeks ago by stl

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

comment:23 follow-up: ↓ 24 Changed 4 weeks ago by 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;
	}

comment:24 in reply to: ↑ 23 Changed 4 weeks ago by 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().

comment:25 Changed 3 weeks ago by 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.

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as reopened
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.