This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit d5a6d4333b7644336a64d7e50cd6630e03916c05 Author: Bryan Call <[email protected]> AuthorDate: Thu Apr 11 15:29:56 2024 -0700 Fixed asan leak issues with RegexContext (#11184) (cherry picked from commit a39b8f112dc977e0f2d41daa3dc42d2fe5b7cd05) --- src/tsutil/DbgCtl.cc | 16 ++++++++++++- src/tsutil/Regex.cc | 66 +++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/src/tsutil/DbgCtl.cc b/src/tsutil/DbgCtl.cc index da17985893..18112318f2 100644 --- a/src/tsutil/DbgCtl.cc +++ b/src/tsutil/DbgCtl.cc @@ -150,7 +150,7 @@ DbgCtl::_new_reference(char const *tag) DebugInterface *p = DebugInterface::get_instance(); debug_assert(tag != nullptr); - // DbgCtl instances may be declared as static objects in the destructors of objects not destoyed till program exit. + // 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 @@ -158,6 +158,20 @@ DbgCtl::_new_reference(char const *tag) ++_RegistryAccessor::registry_reference_count; + // There is a mutex in the C/C++ runtime that both dlopen() and _cxa_thread_atexit() lock while running. + // Creating a _RegistryAccessor instance locks the registry mutex. If the subsequent code in this function triggers + // the construction of a thread_local variable (with a non-trivial destructor), the following deadlock scenario is + // possible: + // 1. Thread 1 calls a DbgCtl constructor, which locks the registry mutex, but then is suspended. + // 2. Thread 2 calls dlopen() for a plugin, locking the runtime mutex. It then executes the constructor for a + // statically allocated DbgCtl object, which blocks on locking the registry mutex. + // 3. Thread 1 resumes, and calls member functions of the derived class of DebugInterface. If this causes the + // the construction of a thread_local variable with a non-trivial destructor, _cxa_thread_atexit() will be called + // to set up a call of the variable's destructor at thread exit. The call to _cxa_thread_atexit() will block on + // the runtime mutex (held by Thread 2). So Thread 1 holds the registry mutex and is blocked waiting for the + // runtime mutex. And Thread 2 holds the runtime mutex and is blocked waiting for the registry mutex. Deadlock. + // + // This deadlock is avoided by having the thread_local variable register its destruction in a non-thread_local class. _RegistryAccessor ra; auto &d{ra.data()}; diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index faea3b8546..37e4b4337b 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -25,6 +25,8 @@ #include <array> #include <assert.h> +#include <vector> +#include <mutex> //---------------------------------------------------------------------------- namespace @@ -41,7 +43,19 @@ my_free(void *ptr, void * /*caller*/) { free(ptr); } -} // namespace + +class RegexContext; // defined below +class RegexContextCleanup +{ +public: + void push_back(RegexContext *ctx); + ~RegexContextCleanup(); + +private: + std::vector<RegexContext *> _contexts; + std::mutex _mutex; +}; +RegexContextCleanup regex_context_cleanup; //---------------------------------------------------------------------------- class RegexContext @@ -50,13 +64,20 @@ public: static RegexContext * get_instance() { - if (!_regex_context) { + if (_shutdown == true) { + return nullptr; + } + + if (_regex_context == nullptr) { _regex_context = new RegexContext(); + regex_context_cleanup.push_back(_regex_context); } return _regex_context; } ~RegexContext() { + _shutdown = true; + if (_general_context != nullptr) { pcre2_general_context_free(_general_context); } @@ -100,17 +121,28 @@ private: pcre2_match_context *_match_context = nullptr; pcre2_jit_stack *_jit_stack = nullptr; thread_local static RegexContext *_regex_context; + static bool _shutdown; // flag to indicate destructor was called, so no new instances can be created }; thread_local RegexContext *RegexContext::_regex_context = nullptr; +bool RegexContext::_shutdown = false; //---------------------------------------------------------------------------- -namespace + +RegexContextCleanup::~RegexContextCleanup() { -struct RegexContextCleanup { - ~RegexContextCleanup() { delete RegexContext::get_instance(); } -}; -thread_local RegexContextCleanup cleanup; + std::lock_guard<std::mutex> guard(_mutex); + for (auto ctx : _contexts) { + delete ctx; + } +} +void +RegexContextCleanup::push_back(RegexContext *ctx) +{ + std::lock_guard<std::mutex> guard(_mutex); + _contexts.push_back(ctx); +} + } // namespace //---------------------------------------------------------------------------- @@ -225,13 +257,21 @@ Regex::compile(std::string_view pattern, uint32_t flags) bool Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, uint32_t flags) { + // free the existing compiled regex if there is one if (_code != nullptr) { pcre2_code_free(_code); } + + // get the RegexContext instance - should only be null when shutting down + RegexContext *regex_context = RegexContext::get_instance(); + if (regex_context == nullptr) { + return false; + } + PCRE2_SIZE error_offset; int error_code; _code = pcre2_compile(reinterpret_cast<PCRE2_SPTR>(pattern.data()), pattern.size(), flags, &error_code, &error_offset, - RegexContext::get_instance()->get_compile_context()); + regex_context->get_compile_context()); if (!_code) { erroroffset = error_offset; @@ -265,11 +305,19 @@ Regex::exec(std::string_view subject) const int32_t Regex::exec(std::string_view subject, RegexMatches &matches) const { + // check if there is a compiled regex if (_code == nullptr) { return 0; } + + // get the RegexContext instance - should only be null when shutting down + RegexContext *regex_context = RegexContext::get_instance(); + if (regex_context == nullptr) { + return 0; + } + int count = pcre2_match(_code, reinterpret_cast<PCRE2_SPTR>(subject.data()), subject.size(), 0, 0, matches.get_match_data(), - RegexContext::get_instance()->get_match_context()); + regex_context->get_match_context()); matches.set_size(count);
