From 6d62d01496e0b442c3d60a8f2ff968ac04843ce6 Mon Sep 17 00:00:00 2001 From: Jeremy Rifkin <51220084+jeremy-rifkin@users.noreply.github.com> Date: Tue, 18 Feb 2025 23:34:08 -0600 Subject: [PATCH] Improve locking surrounding dbghelp and lock in the dbghelp demangler --- CMakeLists.txt | 2 +- src/demangle/demangle_with_winapi.cpp | 3 +++ ...elp_syminit_manager.cpp => dbghelp_utils.cpp} | 14 +++++++++++++- ...elp_syminit_manager.hpp => dbghelp_utils.hpp} | 16 ++++++++++++---- src/symbols/symbols_with_dbghelp.cpp | 11 +++++------ src/unwind/unwind_with_dbghelp.cpp | 6 ++---- 6 files changed, 36 insertions(+), 16 deletions(-) rename src/platform/{dbghelp_syminit_manager.cpp => dbghelp_utils.cpp} (81%) rename src/platform/{dbghelp_syminit_manager.hpp => dbghelp_utils.hpp} (57%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0187b09..fb19634 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -138,7 +138,7 @@ target_sources( src/unwind/unwind_with_winapi.cpp src/utils/microfmt.cpp src/utils/utils.cpp - src/platform/dbghelp_syminit_manager.cpp + src/platform/dbghelp_utils.cpp ) target_include_directories( diff --git a/src/demangle/demangle_with_winapi.cpp b/src/demangle/demangle_with_winapi.cpp index 521c115..35a2452 100644 --- a/src/demangle/demangle_with_winapi.cpp +++ b/src/demangle/demangle_with_winapi.cpp @@ -1,6 +1,7 @@ #ifdef CPPTRACE_DEMANGLE_WITH_WINAPI #include "demangle/demangle.hpp" +#include "platform/dbghelp_utils.hpp" #include @@ -13,6 +14,8 @@ namespace cpptrace { namespace detail { std::string demangle(const std::string& name, bool) { + // Dbghelp is is single-threaded, so acquire a lock. + auto lock = get_dbghelp_lock(); char buffer[500]; auto ret = UnDecorateSymbolName(name.c_str(), buffer, sizeof(buffer) - 1, 0); if(ret == 0) { diff --git a/src/platform/dbghelp_syminit_manager.cpp b/src/platform/dbghelp_utils.cpp similarity index 81% rename from src/platform/dbghelp_syminit_manager.cpp rename to src/platform/dbghelp_utils.cpp index 4fb4c60..b99c9aa 100644 --- a/src/platform/dbghelp_syminit_manager.cpp +++ b/src/platform/dbghelp_utils.cpp @@ -2,7 +2,11 @@ #if IS_WINDOWS -#include "platform/dbghelp_syminit_manager.hpp" +#include "platform/dbghelp_utils.hpp" + +#if defined(CPPTRACE_UNWIND_WITH_DBGHELP) \ + || defined(CPPTRACE_GET_SYMBOLS_WITH_DBGHELP) \ + || defined(CPPTRACE_DEMANGLE_WITH_WINAPI) #include "utils/error.hpp" #include "utils/microfmt.hpp" @@ -53,7 +57,15 @@ namespace detail { return syminit_manager; } + std::recursive_mutex dbghelp_lock; + + std::unique_lock get_dbghelp_lock() { + return std::unique_lock{dbghelp_lock}; + } + } } #endif + +#endif diff --git a/src/platform/dbghelp_syminit_manager.hpp b/src/platform/dbghelp_utils.hpp similarity index 57% rename from src/platform/dbghelp_syminit_manager.hpp rename to src/platform/dbghelp_utils.hpp index db8c24e..daa5623 100644 --- a/src/platform/dbghelp_syminit_manager.hpp +++ b/src/platform/dbghelp_utils.hpp @@ -1,12 +1,17 @@ -#ifndef DBGHELP_SYMINIT_MANAGER_HPP -#define DBGHELP_SYMINIT_MANAGER_HPP +#ifndef DBGHELP_UTILS_HPP +#define DBGHELP_UTILS_HPP + +#if defined(CPPTRACE_UNWIND_WITH_DBGHELP) \ + || defined(CPPTRACE_GET_SYMBOLS_WITH_DBGHELP) \ + || defined(CPPTRACE_DEMANGLE_WITH_WINAPI) #include +#include namespace cpptrace { namespace detail { struct dbghelp_syminit_manager { - // The set below contains Windows `HANDLE` objects, `void*` is used to avoid + // The set below contains Windows `HANDLE` objects, `void*` is used to avoid // including the (expensive) Windows header here std::unordered_map cache; @@ -14,9 +19,12 @@ namespace detail { void* init(void* proc); }; - // Thread-safety: Must only be called from symbols_with_dbghelp while the dbghelp_lock lock is held dbghelp_syminit_manager& get_syminit_manager(); + + std::unique_lock get_dbghelp_lock(); } } #endif + +#endif diff --git a/src/symbols/symbols_with_dbghelp.cpp b/src/symbols/symbols_with_dbghelp.cpp index 0ad218e..593ad71 100644 --- a/src/symbols/symbols_with_dbghelp.cpp +++ b/src/symbols/symbols_with_dbghelp.cpp @@ -2,13 +2,12 @@ #include #include "symbols/symbols.hpp" -#include "platform/dbghelp_syminit_manager.hpp" +#include "platform/dbghelp_utils.hpp" #include "binary/object.hpp" #include "utils/common.hpp" #include "utils/error.hpp" #include "options.hpp" -#include #include #include #include @@ -325,8 +324,6 @@ namespace dbghelp { return true; } - std::recursive_mutex dbghelp_lock; - // TODO: Handle backtrace_pcinfo calling the callback multiple times on inlined functions stacktrace_frame resolve_frame(HANDLE proc, frame_ptr addr) { // The get_frame_object_info() ends up being inexpensive, at on my machine @@ -336,7 +333,8 @@ namespace dbghelp { // get_frame_object_info() 0.001-0.002 ms 0.0003-0.0006 ms // At some point it might make sense to make an option to control this. auto object_frame = get_frame_object_info(addr); - const std::lock_guard lock(dbghelp_lock); // all dbghelp functions are not thread safe + // Dbghelp is is single-threaded, so acquire a lock. + auto lock = get_dbghelp_lock(); alignas(SYMBOL_INFO) char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)]; SYMBOL_INFO* symbol = (SYMBOL_INFO*)buffer; symbol->SizeOfStruct = sizeof(SYMBOL_INFO); @@ -421,7 +419,8 @@ namespace dbghelp { } std::vector resolve_frames(const std::vector& frames) { - const std::lock_guard lock(dbghelp_lock); // all dbghelp functions are not thread safe + // Dbghelp is is single-threaded, so acquire a lock. + auto lock = get_dbghelp_lock(); std::vector trace; trace.reserve(frames.size()); diff --git a/src/unwind/unwind_with_dbghelp.cpp b/src/unwind/unwind_with_dbghelp.cpp index 3888364..6e77b5f 100644 --- a/src/unwind/unwind_with_dbghelp.cpp +++ b/src/unwind/unwind_with_dbghelp.cpp @@ -4,10 +4,9 @@ #include "unwind/unwind.hpp" #include "utils/common.hpp" #include "utils/utils.hpp" -#include "platform/dbghelp_syminit_manager.hpp" +#include "platform/dbghelp_utils.hpp" #include -#include #include #include @@ -96,8 +95,7 @@ namespace detail { std::vector trace; // Dbghelp is is single-threaded, so acquire a lock. - static std::mutex mutex; - std::lock_guard lock(mutex); + auto lock = get_dbghelp_lock(); // For some reason SymInitialize must be called before StackWalk64 // Note that the code assumes that // SymInitialize( GetCurrentProcess(), NULL, TRUE ) has