From 2c68360f052f6f146f52d4c0d33ca8e32f22cd4d Mon Sep 17 00:00:00 2001 From: bneradt Date: Wed, 31 Dec 2025 13:20:02 -0600 Subject: [PATCH] Fix DbgCtl use-after-free shutdown crash via leaky singleton The DbgCtl registry was experiencing use-after-free crashes during program shutdown, particularly visible in CI regression tests. The root cause was the undefined destruction order of static objects in C++. Problem: - DbgCtl objects can be static or thread-local with lifetimes spanning program execution - When one compilation unit's DbgCtl destructs and triggers registry cleanup, other compilation units may still have DbgCtl objects with pointers into that registry - Thread exit order is also unpredictable, causing similar issues when thread-local DbgCtl objects destruct This implements the "leaky singleton" pattern where the registry is: - Created on first use - Never destroyed (destructor is now = default) - Memory (~20KB) reclaimed by OS at process exit Fixes: #12776 --- ci/asan_leak_suppression/regression.txt | 3 + include/tsutil/DbgCtl.h | 12 ++-- plugins/esi/test/CMakeLists.txt | 5 ++ .../esi/test/esi_test_leak_suppression.txt | 5 ++ plugins/esi/test/print_funcs.cc | 16 +---- src/tsutil/DbgCtl.cc | 62 ++----------------- 6 files changed, 24 insertions(+), 79 deletions(-) create mode 100644 plugins/esi/test/esi_test_leak_suppression.txt diff --git a/ci/asan_leak_suppression/regression.txt b/ci/asan_leak_suppression/regression.txt index 4e98783f675..b3dfcf83631 100644 --- a/ci/asan_leak_suppression/regression.txt +++ b/ci/asan_leak_suppression/regression.txt @@ -19,5 +19,8 @@ leak:RegressionTest_HostDBProcessor leak:RegressionTest_DNS leak:RegressionTest_UDPNet_echo leak:HttpConfig::startup +# DbgCtl leaky singleton pattern - intentional, see issue #12776. +leak:DbgCtl::_new_reference +leak:_new_reference # libswoc leak:MemArena::make_block diff --git a/include/tsutil/DbgCtl.h b/include/tsutil/DbgCtl.h index dee8d1ebfb7..76cde235f12 100644 --- a/include/tsutil/DbgCtl.h +++ b/include/tsutil/DbgCtl.h @@ -67,12 +67,10 @@ class DbgCtl // DbgCtl() : _ptr{&_No_tag_dummy()} {} - ~DbgCtl() - { - if (_ptr != &_No_tag_dummy()) { - _rm_reference(); - } - } + // Default destructor: the registry uses a "leaky singleton" pattern to avoid + // use-after-free crashes during shutdown due to undefined static destruction + // order. See issue #12776. + ~DbgCtl() = default; // No copying. Only moving from a tagged to a tagless instance allowed. // @@ -153,8 +151,6 @@ class DbgCtl static const _TagData *_new_reference(char const *tag); - static void _rm_reference(); - class _RegistryAccessor; static std::atomic _config_mode; diff --git a/plugins/esi/test/CMakeLists.txt b/plugins/esi/test/CMakeLists.txt index 1aff571bcb8..c9bc8c2d5f7 100644 --- a/plugins/esi/test/CMakeLists.txt +++ b/plugins/esi/test/CMakeLists.txt @@ -22,6 +22,11 @@ macro(ADD_ESI_TEST NAME) add_executable(${NAME} print_funcs.cc ${ARGN}) target_link_libraries(${NAME} PRIVATE esitest) add_catch2_test(NAME ${NAME}_esi COMMAND $) + # Use ASan leak suppression for intentional DbgCtl leaks (issue #12776) + set_tests_properties( + ${NAME}_esi PROPERTIES ENVIRONMENT + "LSAN_OPTIONS=suppressions=${CMAKE_CURRENT_SOURCE_DIR}/esi_test_leak_suppression.txt" + ) endmacro() add_esi_test(test_docnode docnode_test.cc) diff --git a/plugins/esi/test/esi_test_leak_suppression.txt b/plugins/esi/test/esi_test_leak_suppression.txt new file mode 100644 index 00000000000..8a857cbeab8 --- /dev/null +++ b/plugins/esi/test/esi_test_leak_suppression.txt @@ -0,0 +1,5 @@ +# ESI unit test leak suppressions +# DbgCtl leaky singleton pattern - intentional, see issue #12776 +leak:DbgCtl::_new_reference +leak:_new_reference + diff --git a/plugins/esi/test/print_funcs.cc b/plugins/esi/test/print_funcs.cc index abc9894761a..2596a462054 100644 --- a/plugins/esi/test/print_funcs.cc +++ b/plugins/esi/test/print_funcs.cc @@ -63,14 +63,12 @@ class DbgCtl::_RegistryAccessor { public: // No mutex protection, assuming unit test is single threaded. - // static std::map & registry() { static std::map r; return r; } - static inline int ref_count{0}; }; std::atomic DbgCtl::_config_mode{1}; @@ -78,11 +76,9 @@ std::atomic DbgCtl::_config_mode{1}; DbgCtl::_TagData const * DbgCtl::_new_reference(char const *tag) { - ++_RegistryAccessor::ref_count; - auto it{_RegistryAccessor::registry().find(tag)}; if (it == _RegistryAccessor::registry().end()) { - char *s = new char[std::strlen(tag) + 1]; + char *s = new char[std::strlen(tag) + 1]; // Never deleted - leaky singleton pattern. std::strcpy(s, tag); auto r{_RegistryAccessor::registry().emplace(s, true)}; // Tag is always enabled. it = r.first; @@ -90,16 +86,6 @@ DbgCtl::_new_reference(char const *tag) return &(*it); } -void -DbgCtl::_rm_reference() -{ - if (!--_RegistryAccessor::ref_count) { - for (auto &elem : _RegistryAccessor::registry()) { - delete[] elem.first; - } - } -} - bool DbgCtl::_override_global_on() { diff --git a/src/tsutil/DbgCtl.cc b/src/tsutil/DbgCtl.cc index 0263c31139c..cc78dea8b34 100644 --- a/src/tsutil/DbgCtl.cc +++ b/src/tsutil/DbgCtl.cc @@ -54,9 +54,6 @@ DbgCtl::operator=(DbgCtl &&src) return *this; } -// The resistry of fast debug controllers has a ugly implementation to handle the whole-program initialization -// and destruction order problem with C++. -// class DbgCtl::_RegistryAccessor { private: @@ -79,15 +76,8 @@ class DbgCtl::_RegistryAccessor private: Registry() = default; - // Mutex must be locked before this is called. - // - ~Registry() - { - for (auto &elem : map) { - delete[] elem.first; - } - _mtx.unlock(); - } + // Destructor never called - this is a leaky singleton. See issue #12776. + ~Registry() = default; std::mutex _mtx; @@ -115,8 +105,8 @@ class DbgCtl::_RegistryAccessor } } - // This is not static so it can't be called with the registry mutex is unlocked. It should not be called - // after registry is deleted. + // This is not static so it can't be called with the registry mutex unlocked. + // The registry is never deleted (leaky singleton pattern), so it's always safe to call after creation. // Registry & data() @@ -124,20 +114,6 @@ class DbgCtl::_RegistryAccessor return *_registry_instance; } - void - delete_registry() - { - auto r = _registry_instance.load(); - debug_assert(r != nullptr); - _registry_instance = nullptr; - delete r; - _mtx_is_locked = false; - } - - // Reference count of references to Registry. - // - inline static std::atomic registry_reference_count{0}; - private: bool _mtx_is_locked{false}; @@ -152,13 +128,6 @@ DbgCtl::_new_reference(char const *tag) _TagData *new_tag_data{nullptr}; - // DbgCtl instances may be declared as static objects in the destructors of objects not destroyed till program exit. - // So, we must handle the case where the construction of such instances of DbgCtl overlaps with the destruction of - // other instances of DbgCtl. That is why it is important to make sure the reference count is non-zero before - // constructing _RegistryAccessor. The _RegistryAccessor constructor is thereby able to assume that, if it creates - // the Registry, the new Registry will not be destroyed before the mutex in the new Registry is locked. - - ++_RegistryAccessor::registry_reference_count; { _RegistryAccessor ra; @@ -172,7 +141,7 @@ DbgCtl::_new_reference(char const *tag) debug_assert(sz > 0); - char *t = new char[sz + 1]; // Deleted by ~Registry(). + char *t = new char[sz + 1]; // Never deleted (leaky singleton) - reclaimed by OS at process exit. std::memcpy(t, tag, sz + 1); _TagData new_elem{t, false}; @@ -201,30 +170,11 @@ DbgCtl::_new_reference(char const *tag) return new_tag_data; } -void -DbgCtl::_rm_reference() -{ - _RegistryAccessor ra; - - debug_assert(ra.registry_reference_count != 0); - - --ra.registry_reference_count; - - if (0 == ra.registry_reference_count) { - ra.delete_registry(); - } -} - void DbgCtl::update(const std::function &f) { _RegistryAccessor ra; - - if (!ra.registry_reference_count) { - return; - } - - auto &d{ra.data()}; + auto &d{ra.data()}; for (auto &i : d.map) { i.second = f(i.first);