This is an automated email from the ASF dual-hosted git repository.
wkaras 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 b4dcdec7db Avoid race conditions in Regex.cc. (#11416)
b4dcdec7db is described below
commit b4dcdec7db4f8de6ed3f41623faadf8ddc3b7e55
Author: Walt Karas <[email protected]>
AuthorDate: Wed Jun 5 12:41:25 2024 -0400
Avoid race conditions in Regex.cc. (#11416)
---
src/tsutil/DbgCtl.cc | 60 +++++++++++++++++++++++++++++-----------------------
src/tsutil/Regex.cc | 55 ++++++-----------------------------------------
2 files changed, 39 insertions(+), 76 deletions(-)
diff --git a/src/tsutil/DbgCtl.cc b/src/tsutil/DbgCtl.cc
index 99f35a0b1a..0263c31139 100644
--- a/src/tsutil/DbgCtl.cc
+++ b/src/tsutil/DbgCtl.cc
@@ -150,6 +150,8 @@ DbgCtl::_new_reference(char const *tag)
DebugInterface *p = DebugInterface::get_instance();
debug_assert(tag != nullptr);
+ _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
@@ -157,42 +159,46 @@ DbgCtl::_new_reference(char const *tag)
// the Registry, the new Registry will not be destroyed before the mutex in
the new Registry is locked.
++_RegistryAccessor::registry_reference_count;
+ {
+ _RegistryAccessor ra;
- // 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()};
- auto &d{ra.data()};
+ if (auto it = d.map.find(tag); it != d.map.end()) {
+ return &*it;
+ }
- if (auto it = d.map.find(tag); it != d.map.end()) {
- return &*it;
- }
+ auto sz = std::strlen(tag);
- auto sz = std::strlen(tag);
+ debug_assert(sz > 0);
- debug_assert(sz > 0);
+ char *t = new char[sz + 1]; // Deleted by ~Registry().
+ std::memcpy(t, tag, sz + 1);
+ _TagData new_elem{t, false};
- char *t = new char[sz + 1]; // Deleted by ~Registry().
- std::memcpy(t, tag, sz + 1);
- _TagData new_elem{t, p && p->debug_tag_activated(tag)};
+ auto res = d.map.insert(new_elem);
- auto res = d.map.insert(new_elem);
+ debug_assert(res.second);
+
+ new_tag_data = &*res.first;
+ }
+ new_tag_data->second = p && p->debug_tag_activated(tag);
- debug_assert(res.second);
+ // It is important that debug_tag_activated() is NOT called while the ra
object exists, and the registry mutex is
+ // locked. 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), with the registry mutex locked, 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 a function that causes the the
construction of a thread_local variable with a
+ // non-trivial destructor. This causes a call to _cxa_thread_atexit(),
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.
- return &*res.first;
+ return new_tag_data;
}
void
diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc
index c5beb88e5c..7dd02b6fe5 100644
--- a/src/tsutil/Regex.cc
+++ b/src/tsutil/Regex.cc
@@ -51,19 +51,6 @@ my_free(void *ptr, void * /*caller*/)
free(ptr);
}
-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
{
@@ -71,20 +58,11 @@ public:
static RegexContext *
get_instance()
{
- if (_shutdown == true) {
- return nullptr;
- }
-
- if (_regex_context == nullptr) {
- _regex_context = new RegexContext();
- regex_context_cleanup.push_back(_regex_context);
- }
- return _regex_context;
+ thread_local RegexContext ctx;
+ return &ctx;
}
~RegexContext()
{
- _shutdown = true;
-
if (_general_context != nullptr) {
pcre2_general_context_free(_general_context);
}
@@ -123,33 +101,12 @@ private:
_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;
- static bool _shutdown; // flag to indicate destructor
was called, so no new instances can be created
+ 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 RegexContext *RegexContext::_regex_context = nullptr;
-bool RegexContext::_shutdown = false;
-
-//----------------------------------------------------------------------------
-
-RegexContextCleanup::~RegexContextCleanup()
-{
- 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
//----------------------------------------------------------------------------
