boostorg / system

Boost.org system module
http://boost.org/libs/system
36 stars 85 forks source link

Static initialization order problem of `stdcat_mx_holder<>::mx_` #102

Closed trueqbit closed 1 year ago

trueqbit commented 1 year ago

In commit #986efb142035 you switched to using a global static std::mutex when converting a boost::system:error_category to a std::error_category.

This is subject to the static initialization order fiasco when a program sets up a static std::error_category variable based on a boost::system::error_category.

E.g. even this simple program leads to a crash with Visual C++ 17.4.3, because the compiler decides to initialize the static variable ec1 before boost::system::detail::stdcat_mx_holder<>::mx_, and boost::system::error_category::init_stdcat() locks an uninitialized mutex:

#include <system_error>
#include <boost/asio.hpp>

const std::error_category& ec1 = boost::asio::error::get_misc_category();

int main() {
    return 0;
}

Callstack:

    msvcp140d.dll!00007ffd01062c20()    Unknown
    msvcp140d.dll!00007ffd01063155()    Unknown
>   boost_error_category_issue.exe!std::_Mutex_base::lock() Line 50 C++
    boost_error_category_issue.exe!std::lock_guard<std::mutex>::lock_guard<std::mutex>(std::mutex & _Mtx={...}) Line 428    C++
    boost_error_category_issue.exe!boost::system::error_category::init_stdcat() Line 141    C++
    boost_error_category_issue.exe!boost::system::error_category::operator std::error_category const &() Line 193   C++
    boost_error_category_issue.exe!`dynamic initializer for 'ec1''() Line 4 C++
    ucrtbased.dll!00007ffcfe381fb3()    Unknown
    boost_error_category_issue.exe!__scrt_common_main_seh() Line 258    C++
    boost_error_category_issue.exe!__scrt_common_main() Line 331    C++
    boost_error_category_issue.exe!mainCRTStartup(void * __formal=0x0000004e477a9000) Line 17   C++
    kernel32.dll!BaseThreadInitThunk() Unknown
    ntdll.dll!00007ffdc8c2485b()    Unknown
pdimov commented 1 year ago

The constructor of std::mutex is constexpr, so this shouldn't be possible - in principle - but MSVC has been known to not respect this. I'll see what I can do.

trueqbit commented 1 year ago

Seems like a stupid problem that's surfacing now.

This is suboptimal, but how about wrapping the std::mutex in a function call, for Microsoft's STL only of course? I know I could do that in my programs too. The advantage of doing it in the library would be to avoid surprises, and it has a very low runtime cost - it's not like you'd convert millions of boost::system::error_categoryS to std::error_categoryS in a hot code path.

Something along these lines:

#if defined(_MSVC_STL_VERSION) && (_MSVC_STL_VERSION < 144)
template<class = void> std::mutex& stdcat_mx() noexcept
{
    static std::mutex mx;
    return mx;
}
#else
template<class = void> struct stdcat_mx_holder
{
    static std::mutex mx_;
};

template<class T> std::mutex stdcat_mx_holder<T>::mx_;

inline std::mutex& stdcat_mx() noexcept
{
    return stdcat_mx_holder<>::mx_;
}
#endif
pdimov commented 1 year ago

I suppose that's possible, yes. I had something different in mind - use std::shared_mutex under MSVC, because it's an SRWLock and actually has a constexpr default constructor.

This doesn't work for VS 2013, though, where I might need to apply your suggestion. (Or just mark the test as failed and pretend VS 2013 doesn't exist.)

trueqbit commented 1 year ago

Oh, that's a neat trick too!

I don't care about VS 2013, but I don't know about the boost policies. The question is how much all these legacy compilers are in use. vcpkg, for example, requires VC 2015 Update 3 as a minimal base C++14 compiler, which is problematic enough these days. Anyway, you could do a cascading fallback.

In any case, I would use an inline/template method as an additional abstraction layer that returns a lock_guard instead of the mutex. This encapsulates things very well for the init_stdcat() function, where you don't have to worry about the mutex type.

pdimov commented 1 year ago

Should be fixed on develop now for msvc-12.0 as well, by https://github.com/boostorg/system/commit/71ee26c1888bf3ae57f173e2b41b322a36add823.

pdimov commented 1 year ago

Unless you have something further to add, I consider this fixed. Thanks for the report.

trueqbit commented 1 year ago

Thx Peter for your outstanding response! I can hardly wait for Boost 1.82 :)