bryancall commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4066777398
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1147,3 +1152,260 @@ TEST_CASE("Regex copies answer the same as their
original", "[libts][Regex][copy
CHECK(original.exec(ordinary, original_ordinary) == copy.exec(ordinary,
copy_ordinary));
}
}
+
+namespace
+{
+/** Does PCRE2 have JIT code for this pattern?
+ *
+ * The two tests below are about the JIT stack, and PCRE2 consults it only
when it
+ * has JIT code to run. Without it both a blank context and the shared one
take the
+ * interpreter and return the same answer, so the tests would pass whether or
not
+ * the behaviour they describe is present. Ask PCRE2 rather than assume.
+ *
+ * This asks about one pattern, deliberately: PCRE2 declines some pattern
items and
+ * leaves PCRE2_INFO_JITSIZE at zero on a build whose JIT is otherwise fine,
so the
+ * library-wide probes (pcre2_config, PCRE2_JIT_TEST_ALLOC) answer a different
+ * question. Call it from the test thread only; Catch2 assertions are not
thread safe.
+ */
+bool
+pattern_has_jit(char const *pattern)
+{
+ int errnum = 0;
+ PCRE2_SIZE erroffset = 0;
+ pcre2_code *code = pcre2_compile(reinterpret_cast<PCRE2_SPTR>(pattern),
PCRE2_ZERO_TERMINATED, 0, &errnum, &erroffset, nullptr);
+
+ // A pattern that will not compile is a broken test, not a build without a
JIT.
+ // Reporting it as "no JIT" would turn a typo into a silent skip.
+ REQUIRE(code != nullptr);
+
+ pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);
+ size_t jit_size = 0;
+
+ pcre2_pattern_info(code, PCRE2_INFO_JITSIZE, &jit_size);
+ pcre2_code_free(code);
+ return jit_size > 0;
+}
+} // namespace
+
+// A caller-supplied RegexMatchContext must behave like the shared context that
+// Regex::exec uses when none is supplied. A context built from scratch
silently
+// drops everything the shared one configures, which is how regex_remap came to
+// run with PCRE2's fallback 32KiB JIT stack instead of the 1MiB one.
+TEST_CASE("RegexMatchContext matches the shared context",
"[libts][Regex][RegexMatchContext]")
+{
+ // Quantified alternation of capture groups: every subject character pushes a
+ // backtracking frame, so the JIT stack size is what bounds this.
+ char const *const pattern = R"(^(?:(a)|(b))+$)";
+ if (!pattern_has_jit(pattern)) {
+ SKIP("PCRE2 has no JIT for this pattern, so the JIT stack is never
consulted");
+ }
+
+ Regex re;
+ REQUIRE(re.compile(pattern));
+
+ // 40 bytes of JIT stack per subject character, so PCRE2's 32KiB fallback
stops at
+ // 818 characters and the shared context's 1MiB stack at 26,213. 5,000 sits
six
+ // times above the first and five times below the second, so a blank context
and
+ // the shared one give different answers with margin either way.
+ std::string const subject(5000, 'a');
+
+ RegexMatches shared_matches;
+ RegexMatchContext match_context;
+ RegexMatches own_matches;
+
+ int const shared_rc = re.exec(subject, shared_matches);
+ int const own_rc = re.exec(subject, own_matches, 0, &match_context);
+ CAPTURE(shared_rc, own_rc);
+
+ REQUIRE(shared_rc > 0);
+ REQUIRE(own_rc == shared_rc);
+
+ // The copy paths must carry the inherited configuration too. Building a
blank
+ // context in either of them is the same defect, and nothing else here
notices.
+ RegexMatchContext const copied{match_context};
+ RegexMatchContext assigned;
+
+ assigned = match_context;
Review Comment:
Fixed. `operator=` releases the old context before taking the new one as of
`e4cafab`, which is the leak this flagged, and `9e73cb6` adds the OOM case on
top so a failing copy leaves the object untouched rather than empty.
##########
src/tsutil/Regex.cc:
##########
@@ -257,8 +310,20 @@ struct RegexMatchContext::_MatchContext {
//----------------------------------------------------------------------------
RegexMatchContext::RegexMatchContext()
{
- auto ctx = pcre2_match_context_create(nullptr);
- debug_assert_message(ctx, "Failed to allocate custom pcre2 match context");
+ // Copy the shared context rather than building a blank one. A blank context
+ // silently drops everything the shared context configures, which is how this
+ // type came to run with PCRE2's fallback 32KiB JIT stack instead of the 1MiB
+ // one. Callers override only the fields they mean to.
+ //
+ // pcre2_match_context_copy dereferences its argument rather than returning
null
+ // for one, and RegexContext's constructor does not check its allocations,
so a
+ // shared context that failed to allocate would crash here. Fall back to a
blank
+ // context, which is what this constructor built before and which no reader
of
+ // _match_context dereferences unchecked.
+ auto *shared = RegexContext::get_instance()->get_match_context();
+ auto *ctx = shared != nullptr ? pcre2_match_context_copy(shared) :
pcre2_match_context_create(nullptr);
Review Comment:
This one was right and it was self inflicted. Fixed in `d5029bc`.
The old constructor called `pcre2_match_context_create(nullptr)` and touched
no thread-local. Copying the shared context meant calling
`RegexContext::get_instance()`, which constructs a `thread_local` whose
destructor registers through `__cxa_thread_atexit` and takes the dynamic loader
lock. That is the exact inversion documented above `jit_stack_key` that this
change exists to avoid, and the constructor reintroduced it for any thread that
builds a context without matching, including a plugin doing so during static
initialization while `dlopen` already holds the lock.
Nothing the shared context carries is actually needed there. The JIT stack
callback is thread independent because it resolves its stack through the
pthread key, so the constructor now assigns the callback directly and gets the
1MiB stack without the thread-local. A null general context is what it used
before this branch. The copy and its null-shared fallback are gone, so the
constructor is shorter than the version it replaces.
--
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]
