Abstract out an de-duplicate syminit and symcleanup logic for dbghelp

src/platform/dbghelp_utils.cpp

@@ -10,6 +10,7 @@
 
 #include "utils/error.hpp"
 #include "utils/microfmt.hpp"
+#include "utils/utils.hpp"
 
 #include <unordered_map>
 
@@ -21,40 +22,105 @@
 
 namespace cpptrace {
 namespace detail {
+    dbghelp_syminit_info::dbghelp_syminit_info(void* handle, bool should_sym_cleanup, bool should_close_handle)
+        : handle(handle), should_sym_cleanup(should_sym_cleanup), should_close_handle(should_close_handle) {}
 
-    dbghelp_syminit_manager::~dbghelp_syminit_manager() {
+    dbghelp_syminit_info::~dbghelp_syminit_info() {
-        for(auto kvp : cache) {
+        release();
-            if(!SymCleanup(kvp.second)) {
+    }
-                ASSERT(false, microfmt::format("Cpptrace SymCleanup failed with code {}\n", GetLastError()).c_str());
+
+    void dbghelp_syminit_info::release() {
+        if(!handle) {
+            return;
+        }
+        if(should_sym_cleanup) {
+            if(!SymCleanup(handle)) {
+                throw internal_error("SymCleanup failed with code {}\n", GetLastError());
             }
-            if(!CloseHandle(kvp.second)) {
+        }
-                ASSERT(false, microfmt::format("Cpptrace CloseHandle failed with code {}\n", GetLastError()).c_str());
+        if(should_close_handle) {
+            if(!CloseHandle(handle)) {
+                throw internal_error("CloseHandle failed with code {}\n", GetLastError());
             }
         }
     }
 
-    HANDLE dbghelp_syminit_manager::init(HANDLE proc) {
+    dbghelp_syminit_info dbghelp_syminit_info::make_not_owned(void* handle) {
-        auto itr = cache.find(proc);
+        return dbghelp_syminit_info(handle, false, false);
-
-        if(itr != cache.end()) {
-            return itr->second;
-        }
-        HANDLE duplicated_handle = nullptr;
-        if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-            throw internal_error("DuplicateHandle failed {}", GetLastError());
-        }
-
-        if(!SymInitialize(duplicated_handle, NULL, TRUE)) {
-            throw internal_error("SymInitialize failed {}", GetLastError());
-        }
-        cache[proc] = duplicated_handle;
-        return duplicated_handle;
     }
 
-    // Thread-safety: Must only be called from symbols_with_dbghelp while the dbghelp_lock lock is held
+    dbghelp_syminit_info dbghelp_syminit_info::make_owned(void* handle, bool should_close_handle) {
-    dbghelp_syminit_manager& get_syminit_manager() {
+        return dbghelp_syminit_info(handle, true, should_close_handle);
-        static dbghelp_syminit_manager syminit_manager;
+    }
-        return syminit_manager;
+
+    dbghelp_syminit_info::dbghelp_syminit_info(dbghelp_syminit_info&& other) {
+        handle = exchange(other.handle, nullptr);
+        should_sym_cleanup = other.should_sym_cleanup;
+        should_close_handle = other.should_close_handle;
+    }
+
+    dbghelp_syminit_info& dbghelp_syminit_info::operator=(dbghelp_syminit_info&& other) {
+        release();
+        handle = exchange(other.handle, nullptr);
+        should_sym_cleanup = other.should_sym_cleanup;
+        should_close_handle = other.should_close_handle;
+        return *this;
+    }
+
+    void* dbghelp_syminit_info::get_process_handle() const {
+        return handle;
+    }
+
+    dbghelp_syminit_info dbghelp_syminit_info::make_non_owning_view() const {
+        return make_not_owned(handle);
+    }
+
+    std::unordered_map<HANDLE, dbghelp_syminit_info>& get_syminit_cache() {
+        static std::unordered_map<HANDLE, dbghelp_syminit_info> syminit_cache;
+        return syminit_cache;
+    }
+
+    dbghelp_syminit_info ensure_syminit() {
+        auto lock = get_dbghelp_lock(); // locking around the entire access of the cache unordered_map
+        HANDLE proc = GetCurrentProcess();
+        if(get_cache_mode() == cache_mode::prioritize_speed) {
+            auto& syminit_cache = get_syminit_cache();
+            auto it = syminit_cache.find(proc);
+            if(it != syminit_cache.end()) {
+                return it->second.make_non_owning_view();
+            }
+        }
+
+        auto duplicated_handle = raii_wrap<void*>(nullptr, [] (void* handle) {
+            if(handle) {
+                if(!CloseHandle(handle)) {
+                    throw internal_error("CloseHandle failed with code {}\n", GetLastError());
+                }
+            }
+        });
+        // https://github.com/jeremy-rifkin/cpptrace/issues/204
+        // https://github.com/jeremy-rifkin/cpptrace/pull/206
+        // https://learn.microsoft.com/en-us/windows/win32/debug/initializing-the-symbol-handler
+        // Apparently duplicating the process handle is the idiomatic thing to do and this avoids issues of
+        // SymInitialize being called twice.
+        // TODO: Fallback on failure
+        if(!DuplicateHandle(proc, proc, proc, &duplicated_handle.get(), 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+            throw internal_error("DuplicateHandle failed{}", GetLastError());
+        }
+        if(!SymInitialize(duplicated_handle.get(), NULL, TRUE)) {
+            throw internal_error("SymInitialize failed{}", GetLastError());
+        }
+
+        auto info = dbghelp_syminit_info::make_owned(exchange(duplicated_handle.get(), nullptr), true);
+        // either cache and return a view or return the owning wrapper
+        if(get_cache_mode() == cache_mode::prioritize_speed) {
+            auto& syminit_cache = get_syminit_cache();
+            auto pair = syminit_cache.insert({proc, std::move(info)});
+            VERIFY(pair.second);
+            return pair.first->second.make_non_owning_view();
+        } else {
+            return info;
+        }
     }
 
     std::recursive_mutex dbghelp_lock;

src/platform/dbghelp_utils.hpp

@@ -10,16 +10,34 @@
 
 namespace cpptrace {
 namespace detail {
-    struct dbghelp_syminit_manager {
+    class dbghelp_syminit_info {
-        // The set below contains Windows `HANDLE` objects, `void*` is used to avoid
+        // `void*` is used to avoid including the (expensive) windows.h header here
-        // including the (expensive) Windows header here
+        void* handle = nullptr;
-        std::unordered_map<void*, void*> cache;
+        bool should_sym_cleanup; // true if cleanup is not managed by the syminit cache
+        bool should_close_handle; // true if cleanup is not managed by the syminit cache and the handle was duplicated
+        dbghelp_syminit_info(void* handle, bool should_sym_cleanup, bool should_close_handle);
+    public:
+        ~dbghelp_syminit_info();
+        void release();
 
-        ~dbghelp_syminit_manager();
+        static dbghelp_syminit_info make_not_owned(void* handle);
-        void* init(void* proc);
+        static dbghelp_syminit_info make_owned(void* handle, bool should_close_handle);
+
+        dbghelp_syminit_info(const dbghelp_syminit_info&) = delete;
+        dbghelp_syminit_info(dbghelp_syminit_info&&);
+        dbghelp_syminit_info& operator=(const dbghelp_syminit_info&) = delete;
+        dbghelp_syminit_info& operator=(dbghelp_syminit_info&&);
+
+        void* get_process_handle() const;
+        dbghelp_syminit_info make_non_owning_view() const;
     };
 
-    dbghelp_syminit_manager& get_syminit_manager();
+    // Ensure SymInitialize is called on the process. This function either
+    // - Finds that SymInitialize has been called for a handle to the current process already, in which case it returns
+    //   a non-owning dbghelp_syminit_info instance holding the handle
+    // - Calls SymInitialize a handle to the current process, caches it, and returns a non-owning dbghelp_syminit_info
+    // - Calls SymInitialize and returns an owning dbghelp_syminit_info which will handle cleanup
+    dbghelp_syminit_info ensure_syminit();
 
     std::unique_lock<std::recursive_mutex> get_dbghelp_lock();
 }

src/symbols/symbols_with_dbghelp.cpp

@@ -427,21 +427,10 @@ namespace dbghelp {
         // TODO: When does this need to be called? Can it be moved to the symbolizer?
         SymSetOptions(SYMOPT_ALLOW_ABSOLUTE_SYMBOLS);
 
-        HANDLE duplicated_handle  = nullptr;
+        auto syminit_info = ensure_syminit();
-        HANDLE proc = GetCurrentProcess();
-        if(get_cache_mode() == cache_mode::prioritize_speed) {
-            duplicated_handle = get_syminit_manager().init(proc);
-        } else {
-            if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-                throw internal_error("DuplicateHandle failed {}", GetLastError());
-            }
-            if(!SymInitialize(duplicated_handle, NULL, TRUE)) {
-                throw internal_error("SymInitialize failed {}", GetLastError());
-            }
-        }
         for(const auto frame : frames) {
             try {
-                trace.push_back(resolve_frame(duplicated_handle , frame));
+                trace.push_back(resolve_frame(syminit_info.get_process_handle() , frame));
             } catch(...) { // NOSONAR
                 if(!detail::should_absorb_trace_exceptions()) {
                     throw;
@@ -451,14 +440,6 @@ namespace dbghelp {
                 trace.push_back(entry);
             }
         }
-        if(get_cache_mode() != cache_mode::prioritize_speed) {
-            if(!SymCleanup(duplicated_handle)) {
-                throw internal_error("SymCleanup failed");
-            }
-            if(!CloseHandle(duplicated_handle)) {
-                throw internal_error("CloseHandle failed");
-            }
-        }
         return trace;
     }
 }

src/unwind/unwind_with_dbghelp.cpp

@@ -101,24 +101,13 @@ namespace detail {
         // SymInitialize( GetCurrentProcess(), NULL, TRUE ) has
         // already been called.
         //
-        HANDLE duplicated_handle = nullptr;
+        auto syminit_info = ensure_syminit();
-        HANDLE proc = GetCurrentProcess();
         HANDLE thread = GetCurrentThread();
-        if(get_cache_mode() == cache_mode::prioritize_speed) {
-            duplicated_handle = get_syminit_manager().init(proc);
-        } else {
-            if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-                throw internal_error("DuplicateHandle failed{}", GetLastError());
-            }
-            if(!SymInitialize(duplicated_handle, NULL, TRUE)) {
-                throw internal_error("SymInitialize failed{}", GetLastError());
-            }
-        }
         while(trace.size() < max_depth) {
             if(
                 !StackWalk64(
                     machine_type,
-                    duplicated_handle,
+                    syminit_info.get_process_handle(),
                     thread,
                     &frame,
                     machine_type == IMAGE_FILE_MACHINE_I386 ? NULL : &context,
@@ -146,14 +135,6 @@ namespace detail {
                 break;
             }
         }
-        if(get_cache_mode() != cache_mode::prioritize_speed) {
-            if(!SymCleanup(duplicated_handle)) {
-                throw internal_error("SymCleanup failed");
-            }
-            if(!CloseHandle(duplicated_handle)) {
-                throw internal_error("CloseHandle failed");
-            }
-        }
         return trace;
     }