Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
trueqbit opened this issue Jan 18, 2023 · 7 comments
Closed

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

trueqbit opened this issue Jan 18, 2023 · 7 comments

Comments

@trueqbit
Copy link

trueqbit commented Jan 18, 2023

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
Copy link
Member

pdimov commented Jan 18, 2023

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
Copy link
Author

trueqbit commented Jan 19, 2023

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
Copy link
Member

pdimov commented Jan 19, 2023

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
Copy link
Author

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
Copy link
Member

pdimov commented Jan 20, 2023

Should be fixed on develop now for msvc-12.0 as well, by 71ee26c.

@pdimov
Copy link
Member

pdimov commented Jan 22, 2023

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

@pdimov pdimov closed this as completed Jan 22, 2023
@trueqbit
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants