This is an automated email from the ASF dual-hosted git repository. wkaras pushed a commit to branch pcre_deadlock in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 04d4c0fcef00e82a33b5cd2cc2cbb91c941cb990 Author: Walt Karas <[email protected]> AuthorDate: Fri Mar 15 00:49:19 2024 +0000 RegexContext instance created in the heap for each thread is leaked. Damien found by running an ASAN build that the RegexContext instance created in the heap for each thread is leaked. The thread_local RegexContextCleanup variables are never constructed becasue they are never referenced in any thread. The solution is a different approach to avoiding deadlock when loading plugins with statically allocated DbgCtl instances. Construction of thread_locals is avoided while the DbgCtl registry mutex is locked. --- include/tscore/DiagsTypes.h | 2 ++ include/tsutil/DbgCtl.h | 8 ++++++++ include/tsutil/Regex.h | 2 ++ src/tscore/Diags.cc | 6 ++++++ src/tsutil/DbgCtl.cc | 23 +++++++++++++++++++++++ src/tsutil/Regex.cc | 43 ++++++++++++++++--------------------------- 6 files changed, 57 insertions(+), 27 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index c7616fa8e0..969427a0d9 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -156,6 +156,8 @@ public: return unlikely(this->on(mode)) && tag_activated(tag, mode); } + void cons_thread_local() override; + ///////////////////////////////////// // low-level tag inquiry functions // ///////////////////////////////////// diff --git a/include/tsutil/DbgCtl.h b/include/tsutil/DbgCtl.h index 5513c22d57..8ef41e08d4 100644 --- a/include/tsutil/DbgCtl.h +++ b/include/tsutil/DbgCtl.h @@ -45,6 +45,14 @@ public: virtual void print_va(const char *debug_tag, DiagsLevel diags_level, const SourceLocation *loc, const char *format_string, va_list ap) const = 0; + // Trigger the construction of all statically allocated thread_local variables used by the instance of the derived + // class, which have non-trivial destructors. This is necessary to avoid a potential deadlock condition when loading + // a plugin. + virtual void + cons_thread_local() + { + } + static DebugInterface *get_instance(); static void set_instance(DebugInterface *); diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index c4ca8feb03..4b68cbc328 100644 --- a/include/tsutil/Regex.h +++ b/include/tsutil/Regex.h @@ -134,6 +134,8 @@ public: /// @return The number of capture groups in the compiled pattern. int get_capture_count(); + static void cons_thread_local(); + private: // @internal - Because the PCRE header is badly done, we can't forward declare the PCRE // enough to use as pointers. For some reason the header defines in name only a struct and diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index 7aa63546f0..4690de52a3 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -314,6 +314,12 @@ Diags::print_va(const char *debug_tag, DiagsLevel diags_level, const SourceLocat #endif } +void +Diags::cons_thread_local() +{ + Regex::cons_thread_local(); +} + ////////////////////////////////////////////////////////////////////////////// // // bool Diags::tag_activated(char * tag, DiagsTagType mode) diff --git a/src/tsutil/DbgCtl.cc b/src/tsutil/DbgCtl.cc index da17985893..a5f98248ce 100644 --- a/src/tsutil/DbgCtl.cc +++ b/src/tsutil/DbgCtl.cc @@ -158,6 +158,26 @@ DbgCtl::_new_reference(char const *tag) ++_RegistryAccessor::registry_reference_count; + // It seems 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 calling this function, which must make sure all thread_local variables used by + // the instance of the derived class of DebugInterface are constructed. + // + if (p) { + p->cons_thread_local(); + } + _RegistryAccessor ra; auto &d{ra.data()}; @@ -285,6 +305,9 @@ DebugInterface::get_instance() void DebugInterface::set_instance(DebugInterface *i) { + // See comments in DbgCtl::_new_reference(). + i->cons_thread_local(); + di_inst = i; DbgCtl::update([&](const char *t) { return i->debug_tag_activated(t); }); } diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index faea3b8546..30292514d4 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -41,19 +41,17 @@ my_free(void *ptr, void * /*caller*/) { free(ptr); } -} // namespace -//---------------------------------------------------------------------------- class RegexContext { public: - static RegexContext * - get_instance() + RegexContext() { - if (!_regex_context) { - _regex_context = new RegexContext(); - } - return _regex_context; + _general_context = pcre2_general_context_create(my_malloc, my_free, nullptr); + _compile_context = pcre2_compile_context_create(_general_context); + _match_context = pcre2_match_context_create(_general_context); + _jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max + pcre2_jit_stack_assign(_match_context, nullptr, _jit_stack); } ~RegexContext() { @@ -87,32 +85,23 @@ public: } private: - RegexContext() - { - _general_context = pcre2_general_context_create(my_malloc, my_free, nullptr); - _compile_context = pcre2_compile_context_create(_general_context); - _match_context = pcre2_match_context_create(_general_context); - _jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max - pcre2_jit_stack_assign(_match_context, nullptr, _jit_stack); - } pcre2_general_context *_general_context = nullptr; pcre2_compile_context *_compile_context = nullptr; pcre2_match_context *_match_context = nullptr; pcre2_jit_stack *_jit_stack = nullptr; - thread_local static RegexContext *_regex_context; }; -thread_local RegexContext *RegexContext::_regex_context = nullptr; +thread_local RegexContext regex_context; -//---------------------------------------------------------------------------- -namespace -{ -struct RegexContextCleanup { - ~RegexContextCleanup() { delete RegexContext::get_instance(); } -}; -thread_local RegexContextCleanup cleanup; } // namespace +void +Regex::cons_thread_local() +{ + // Referencing it will cause it to be constructed in the executing thread. + static_cast<void>(regex_context); +} + //---------------------------------------------------------------------------- RegexMatches::RegexMatches(uint32_t size) { @@ -231,7 +220,7 @@ Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, u 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; @@ -269,7 +258,7 @@ Regex::exec(std::string_view subject, RegexMatches &matches) const 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);
