This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new a39b8f112d Fixed asan leak issues with RegexContext (#11184)
a39b8f112d is described below
commit a39b8f112dc977e0f2d41daa3dc42d2fe5b7cd05
Author: Bryan Call <[email protected]>
AuthorDate: Thu Apr 11 15:29:56 2024 -0700
Fixed asan leak issues with RegexContext (#11184)
---
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);
