bryancall commented on code in PR #13661:
URL: https://github.com/apache/trafficserver/pull/13661#discussion_r3984320063
##########
src/tsutil/Regex.cc:
##########
@@ -79,6 +80,46 @@ 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;
+pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT;
+
+void
+destroy_jit_stack(void *stack)
+{
+ if (stack != nullptr) {
+ pcre2_jit_stack_free(static_cast<pcre2_jit_stack *>(stack));
+ }
+}
+
+void
+make_jit_stack_key()
+{
+ pthread_key_create(&jit_stack_key, destroy_jit_stack);
Review Comment:
Valid, fixed in fff1a840a2.
The dangerous part is not that the stack would be missing, it is that
`jit_stack_key` is
a namespace scope `pthread_key_t` and therefore zero initialized. Key 0 can
be a live key
belonging to another subsystem, so a failed create would have had the
callback return
that key's value and hand it to PCRE2 as a JIT stack.
The callback now records whether the key was created and returns null when
it was not.
`pcre2jit` documents a null return as thread safe, so the match falls back
to PCRE2's own
default stack: degraded, but correct, and the same behaviour this plugin had
before the
change.
The write to that flag happens inside the `pthread_once` initializer, so it
is ordered
before every reader.
Verified on a dev-asan build: unit suite 134 of 134, regex_remap AuTest
passes with the
3KB URL returning 301 and one -46 plus one -47, and no leaked JIT stack.
--
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]
