bryancall commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4087655608
##########
src/tsutil/Regex.cc:
##########
@@ -278,22 +351,61 @@ RegexMatchContext::operator=(RegexMatchContext const
&other)
{
if (&other != this) {
auto ptr = _MatchContext::get(other._match_context);
- if (nullptr != ptr) {
- pcre2_match_context *const ctx = pcre2_match_context_copy(ptr);
- _MatchContext::set(_match_context, ctx);
- } else {
- _MatchContext::set(_match_context, nullptr);
+
+ // Take the copy before releasing what this object already holds, so a
failing
+ // copy leaves it holding its old context rather than a freed one.
Releasing it
+ // is what this operator used to omit, and every assignment leaked one
context.
+ pcre2_match_context *const ctx = nullptr != ptr ?
pcre2_match_context_copy(ptr) : nullptr;
+
+ // pcre2_match_context_copy() returns nullptr when it cannot allocate.
Assigning it
+ // anyway would free the old context and leave this object empty, which is
the very
+ // thing the ordering above exists to prevent, so leave the object
untouched instead.
+ if (nullptr != ptr && nullptr == ctx) {
+ return *this;
}
+
+ pcre2_match_context *const old = _MatchContext::get(_match_context);
+
+ _MatchContext::set(_match_context, ctx);
+ if (old != nullptr) {
+ pcre2_match_context_free(old);
+ }
+ }
+ return *this;
+}
+
+//----------------------------------------------------------------------------
+RegexMatchContext::RegexMatchContext(RegexMatchContext &&that) noexcept
+{
+ // Through the typed accessors rather than std::exchange on the raw member:
_ptr is
+ // void *, and void * does not implicitly convert to pcre2_match_context *,
which is
+ // what set() takes.
+ _MatchContext::set(_match_context, _MatchContext::get(that._match_context));
+ _MatchContext::set(that._match_context, nullptr);
+}
+
+//----------------------------------------------------------------------------
+RegexMatchContext &
+RegexMatchContext::operator=(RegexMatchContext &&that) noexcept
+{
+ if (this != &that) {
+ if (auto *const old = _MatchContext::get(_match_context); old != nullptr) {
+ pcre2_match_context_free(old);
+ }
+ _MatchContext::set(_match_context,
_MatchContext::get(that._match_context));
+ _MatchContext::set(that._match_context, nullptr);
}
return *this;
}
//----------------------------------------------------------------------------
RegexMatchContext::~RegexMatchContext()
{
- auto ptr = _MatchContext::get(_match_context);
- debug_assert_message(ptr, "Failed to get the match context");
- if (ptr != nullptr) {
+ // No assert that the pointer is set. Null is now a legitimate state: a
moved-from
+ // object holds nothing, and asserting here would abort a debug build on the
first
+ // destruction of one. Before the move operations existed the only way to
reach this
+ // with null was a failed construction, which is why the assert was
reasonable then.
Review Comment:
Agreed. Cut to one line that describes the current state, "A moved-from
object holds no context," in 584075033f27.
##########
src/tsutil/Regex.cc:
##########
@@ -79,6 +80,67 @@ my_free(void *ptr, void * /*caller*/)
free(ptr);
}
+//----------------------------------------------------------------------------
+// One match context is shared by every thread that matches through it, and
PCRE2
+// requires a distinct JIT stack per thread, so the stack comes from a callback
+// invoked at match time rather than a pointer baked in when the context is
built.
+//
+// The per thread stack is held in a pthread key rather than a thread_local. A
+// thread_local with a destructor registers it through __cxa_thread_atexit,
which
+// takes the dynamic loader lock; doing that from a match would invert lock
order
+// against a dlopen caller running a plugin's static initialization. See the
same
+// hazard described at Diags::tag_activated. A pthread key registers its
destructor
+// once, at key creation, and never from the matching path.
+pthread_key_t jit_stack_key;
+bool jit_stack_key_valid = false;
+pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT;
Review Comment:
They already do. They sit inside the unnamed namespace that opens at line 68
and closes at line 198, which is too far above this hunk to show in the diff.
`nm` on the built object lists `jit_stack_key`, `jit_stack_key_valid` and
`jit_stack_key_once` as local symbols under `(anonymous namespace)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
