From b24661304510f3968e98d58815806044b1061f1c Mon Sep 17 00:00:00 2001 From: Jeremy <51220084+jeremy-rifkin@users.noreply.github.com> Date: Sun, 23 Jul 2023 17:20:27 -0400 Subject: [PATCH] Thread safety --- src/demangle/demangle_with_cxxabi.cpp | 1 + src/full/full_trace_with_libbacktrace.cpp | 4 ++ src/platform/program_name.hpp | 49 ++++++++++++++++------- src/symbols/symbols_with_addr2line.cpp | 7 +++- src/symbols/symbols_with_dbghelp.cpp | 3 ++ src/symbols/symbols_with_dl.cpp | 2 +- src/symbols/symbols_with_libbacktrace.cpp | 4 ++ src/unwind/unwind_with_execinfo.cpp | 2 +- src/unwind/unwind_with_unwind.cpp | 3 +- 9 files changed, 56 insertions(+), 19 deletions(-) diff --git a/src/demangle/demangle_with_cxxabi.cpp b/src/demangle/demangle_with_cxxabi.cpp index 1296e63..04f33ef 100644 --- a/src/demangle/demangle_with_cxxabi.cpp +++ b/src/demangle/demangle_with_cxxabi.cpp @@ -11,6 +11,7 @@ namespace cpptrace { namespace detail { std::string demangle(const std::string& name) { int status; + // presumably thread-safe char* demangled = abi::__cxa_demangle(name.c_str(), nullptr, nullptr, &status); if(demangled) { std::string str = demangled; diff --git a/src/full/full_trace_with_libbacktrace.cpp b/src/full/full_trace_with_libbacktrace.cpp index 5db8629..508d699 100644 --- a/src/full/full_trace_with_libbacktrace.cpp +++ b/src/full/full_trace_with_libbacktrace.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #ifdef CPPTRACE_BACKTRACE_PATH @@ -52,7 +53,10 @@ namespace cpptrace { fprintf(stderr, "Libbacktrace error: %s, code %d\n", msg, errnum); } + std::mutex state_mutex; + backtrace_state* get_backtrace_state() { + const std::lock_guard lock(state_mutex); // backtrace_create_state must be called only one time per program // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static backtrace_state* state = nullptr; diff --git a/src/platform/program_name.hpp b/src/platform/program_name.hpp index 9fda208..18913eb 100644 --- a/src/platform/program_name.hpp +++ b/src/platform/program_name.hpp @@ -1,22 +1,38 @@ #ifndef PROGRAM_NAME_HPP #define PROGRAM_NAME_HPP +#include #include +namespace cpptrace { + namespace detail { + inline std::mutex& get_program_name_mutex() { // stupid workaround for an inline variable + static std::mutex mutex; + return mutex; + } + } +} + #if defined(_WIN32) #include namespace cpptrace { namespace detail { inline std::string program_name() { - // TODO: Cache this better - char buffer[MAX_PATH + 1]; - int res = GetModuleFileNameA(nullptr, buffer, MAX_PATH); - if(res) { - return buffer; - } else { - return ""; + const std::lock_guard lock(get_program_name_mutex()); + static std::string name; + static bool did_init = false; + static bool valid = false; + if(!did_init) { + did_init = true; + char buffer[MAX_PATH + 1]; + int res = GetModuleFileNameA(nullptr, buffer, MAX_PATH); + if(res) { + name = buffer; + valid = true; + } } + return valid && !name.empty() ? name.c_str() : nullptr; } } } @@ -30,16 +46,20 @@ namespace cpptrace { namespace cpptrace { namespace detail { inline const char* program_name() { - // TODO: Cache this better + const std::lock_guard lock(get_program_name_mutex()); static std::string name; - if (!name.empty()) { + static bool did_init = false; + static bool valid = false; + if(!did_init) { + did_init = true; std::uint32_t bufferSize = PATH_MAX + 1; char buffer[bufferSize]; - if (_NSGetExecutablePath(buffer, &bufferSize) != 0) - return nullptr; - name.assign(buffer, bufferSize); + if(_NSGetExecutablePath(buffer, &bufferSize) == 0) { + name.assign(buffer, bufferSize); + valid = true; + } } - return name.c_str(); + return valid && !name.empty() ? name.c_str() : nullptr; } } } @@ -53,6 +73,7 @@ namespace cpptrace { namespace cpptrace { namespace detail { inline const char* program_name() { + const std::lock_guard lock(get_program_name_mutex()); static std::string name; static bool did_init = false; static bool valid = false; @@ -67,7 +88,7 @@ namespace cpptrace { name = buffer; valid = true; } - return valid ? name.c_str() : nullptr; + return valid && !name.empty() ? name.c_str() : nullptr; } } } diff --git a/src/symbols/symbols_with_addr2line.cpp b/src/symbols/symbols_with_addr2line.cpp index f536866..b2a259b 100644 --- a/src/symbols/symbols_with_addr2line.cpp +++ b/src/symbols/symbols_with_addr2line.cpp @@ -44,7 +44,7 @@ namespace cpptrace { Dl_info info; dlframe frame; frame.raw_address = reinterpret_cast(addr); - if(dladdr(addr, &info)) { + if(dladdr(addr, &info)) { // thread safe // dli_sname and dli_saddr are only present with -rdynamic, sname will be included // but we don't really need dli_saddr frame.obj_path = info.dli_fname; @@ -153,13 +153,15 @@ namespace cpptrace { dlframe frame; frame.raw_address = reinterpret_cast(addr); HMODULE handle; + // Multithread safe as long as another thread doesn't come along and free the module if(GetModuleHandleExA( GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, static_cast(addr), &handle )) { - fflush(stderr); + fflush(stderr); // TODO: remove char path[MAX_PATH]; + // TODO: Memoize if(GetModuleFileNameA(handle, path, sizeof(path))) { ///fprintf(stderr, "path: %s base: %p\n", path, handle); frame.obj_path = path; @@ -177,6 +179,7 @@ namespace cpptrace { } bool has_addr2line() { + // TODO: Memoize // TODO: Popen is a hack. Implement properly with CreateProcess and pipes later. FILE* p = popen("addr2line --version", "r"); return pclose(p) == 0; diff --git a/src/symbols/symbols_with_dbghelp.cpp b/src/symbols/symbols_with_dbghelp.cpp index e62983a..d7e708d 100644 --- a/src/symbols/symbols_with_dbghelp.cpp +++ b/src/symbols/symbols_with_dbghelp.cpp @@ -299,6 +299,8 @@ namespace cpptrace { return true; } + std::mutex dbghelp_lock; + // TODO: Handle backtrace_pcinfo calling the callback multiple times on inlined functions struct symbolizer::impl { bool good = true; @@ -320,6 +322,7 @@ namespace cpptrace { } stacktrace_frame resolve_frame(void* addr) { + const std::lock_guard lock(dbghelp_lock); // all dbghelp functions are not thread safe alignas(SYMBOL_INFO) char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)]; SYMBOL_INFO* symbol = (SYMBOL_INFO*)buffer; symbol->SizeOfStruct = sizeof(SYMBOL_INFO); diff --git a/src/symbols/symbols_with_dl.cpp b/src/symbols/symbols_with_dl.cpp index 089d4fe..906e036 100644 --- a/src/symbols/symbols_with_dl.cpp +++ b/src/symbols/symbols_with_dl.cpp @@ -15,7 +15,7 @@ namespace cpptrace { // NOLINTNEXTLINE(readability-convert-member-functions-to-static) stacktrace_frame resolve_frame(const void* addr) { Dl_info info; - if(dladdr(addr, &info)) { + if(dladdr(addr, &info)) { // thread-safe return { reinterpret_cast(addr), 0, diff --git a/src/symbols/symbols_with_libbacktrace.cpp b/src/symbols/symbols_with_libbacktrace.cpp index 17ee7df..fc25e51 100644 --- a/src/symbols/symbols_with_libbacktrace.cpp +++ b/src/symbols/symbols_with_libbacktrace.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #ifdef CPPTRACE_BACKTRACE_PATH @@ -41,7 +42,10 @@ namespace cpptrace { fprintf(stderr, "Libbacktrace error: %s, code %d\n", msg, errnum); } + std::mutex state_mutex; + backtrace_state* get_backtrace_state() { + const std::lock_guard lock(state_mutex); // backtrace_create_state must be called only one time per program // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static backtrace_state* state = nullptr; diff --git a/src/unwind/unwind_with_execinfo.cpp b/src/unwind/unwind_with_execinfo.cpp index dd8dbec..eea4d7b 100644 --- a/src/unwind/unwind_with_execinfo.cpp +++ b/src/unwind/unwind_with_execinfo.cpp @@ -14,7 +14,7 @@ namespace cpptrace { CPPTRACE_FORCE_NO_INLINE std::vector capture_frames(size_t skip) { std::vector frames(hard_max_frames + skip, nullptr); - const int n_frames = backtrace(frames.data(), int(hard_max_frames + skip)); + const int n_frames = backtrace(frames.data(), int(hard_max_frames + skip)); // thread safe frames.resize(n_frames); frames.erase(frames.begin(), frames.begin() + ptrdiff_t(std::min(skip + 1, frames.size()))); frames.shrink_to_fit(); diff --git a/src/unwind/unwind_with_unwind.cpp b/src/unwind/unwind_with_unwind.cpp index c4a2f5b..919d3dc 100644 --- a/src/unwind/unwind_with_unwind.cpp +++ b/src/unwind/unwind_with_unwind.cpp @@ -38,6 +38,7 @@ namespace cpptrace { if (ip == uintptr_t(0) || state.count == state.vec.size()) { return _URC_END_OF_STACK; } else { + // TODO: push_back?... state.vec[state.count++] = (void*)ip; return _URC_NO_REASON; } @@ -47,7 +48,7 @@ namespace cpptrace { std::vector capture_frames(size_t skip) { std::vector frames(hard_max_frames, nullptr); unwind_state state{skip + 1, 0, frames}; - _Unwind_Backtrace(unwind_callback, &state); + _Unwind_Backtrace(unwind_callback, &state); // presumably thread-safe frames.resize(state.count); frames.shrink_to_fit(); return frames;