bryancall commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4355468408
##########
src/tsutil/Regex.cc:
##########
@@ -80,6 +81,70 @@ 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:
These already have internal linkage. They sit inside the unnamed namespace
that opens at line 69 and closes at line 199, which is too far above this hunk
to show in the diff. JosiahWI asked the same thing earlier and the answer is in
that thread.
##########
src/tsutil/Regex.cc:
##########
@@ -80,6 +81,70 @@ 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;
+
+void
+destroy_jit_stack(void *stack)
+{
+ if (stack != nullptr) {
+ pcre2_jit_stack_free(static_cast<pcre2_jit_stack *>(stack));
+ }
+}
+
+void
+make_jit_stack_key()
+{
+ jit_stack_key_valid = pthread_key_create(&jit_stack_key, destroy_jit_stack)
== 0;
+}
+
+pcre2_jit_stack *
+jit_stack_for_this_thread(void *)
+{
+ pthread_once(&jit_stack_key_once, make_jit_stack_key);
+ if (!jit_stack_key_valid) {
+ // Without a key there is nowhere to keep a stack, and jit_stack_key holds
a
+ // default value that may name an unrelated key. Returning null tells
PCRE2 to
+ // use its own default stack, which pcre2jit documents as thread safe.
+ return nullptr;
+ }
+
+ auto *stack = static_cast<pcre2_jit_stack
*>(pthread_getspecific(jit_stack_key));
+ if (stack == nullptr) {
+ // One page to start, one mebibyte at most. The maximum is address space
reserved at
+ // creation and made resident only as deep as a match actually goes, so a
larger one
+ // costs nothing per match.
+ //
+ // It does NOT cover every subject a client can send. Measured with the
pattern the
+ // unit tests use, a mebibyte resolves about 26,213 characters, while
+ // proxy.config.http.request_header_max_size defaults to 32768. A longer
subject
+ // fails with PCRE2_ERROR_JIT_STACKLIMIT, which exec() returns as a
negative code.
+ // There is no retreat to a smaller stack and none to the interpreter:
measured at
+ // 26,214 characters and again at 100,000, the match returns -46 rather
than
+ // completing by another route.
+ stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr);
+ if (pthread_setspecific(jit_stack_key, stack) != 0) {
+ // Nothing holds the stack now, so it would leak once per match. Give it
back and
+ // let PCRE2 use its own default stack for this call.
+ pcre2_jit_stack_free(stack);
Review Comment:
Both cases are already safe. If `pcre2_jit_stack_create()` returns null,
`pthread_setspecific()` stores null and this returns null, and a null from the
callback makes PCRE2 use its internal 32 KiB stack for that match
(`pcre2_jit_stack_assign(3)`). The next match on the thread tries to create the
stack again. On the failure branch, `pcre2_jit_stack_free(NULL)` is documented
to return without doing anything. So I've left it as is rather than add a
branch that changes no behavior.
##########
src/tsutil/Regex.cc:
##########
@@ -279,22 +355,58 @@ 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) {
Review Comment:
This is deliberate. A copy assignment has no way to report failure, and when
`pcre2_match_context_copy()` cannot allocate the two outcomes are an object
with no context or an object that keeps the one it had. The comment above the
check explains why it keeps the old one. The trade is that the object keeps its
previous settings instead of `other`'s, and only when allocating a match
context fails.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1646,3 +1651,362 @@ 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;
+
+ RegexMatches copied_matches;
+ RegexMatches assigned_matches;
+
+ REQUIRE(re.exec(subject, copied_matches, 0, &copied) == shared_rc);
+ REQUIRE(re.exec(subject, assigned_matches, 0, &assigned) == shared_rc);
+}
+
+// Ownership only, and deliberately NOT inside the case above.
+//
+// That case SKIPs when pattern_has_jit() is false, which is right for what it
tests and
+// wrong for this: move construction and move assignment have nothing to do
with the JIT.
+// Gating them there meant the only coverage of the double free stopped
running on exactly
+// the builds it was written for, a PCRE2 without JIT or the hardened runtime
where JIT
+// allocation fails. A regression test that disappears on the affected
platform is worse
+// than none, because it reports green.
+TEST_CASE("RegexMatchContext move operations transfer ownership",
"[libts][Regex][RegexMatchContext]")
+{
+ Regex re;
+
+ REQUIRE(re.compile("^(a+)(b+)$"));
+
+ std::string_view const subject{"aaabbb"};
+
+ auto matches_through = [&](RegexMatchContext const *ctx) {
+ RegexMatches m;
+ return re.exec(subject, m, 0, ctx);
+ };
+
+ // Baseline through a plain context, so a later mismatch means the move
broke something
+ // rather than the pattern never having matched.
+ RegexMatchContext plain;
+ int const expected = matches_through(&plain);
+
+ REQUIRE(expected > 0);
+
+ SECTION("move construction")
+ {
+ RegexMatchContext donor;
+ RegexMatchContext const moved{std::move(donor)};
+
+ REQUIRE(matches_through(&moved) == expected);
+ // A double free aborts when donor and moved are both destroyed, so
leaving this
+ // section is itself the second assertion.
+ }
+
+ SECTION("move assignment onto a destination that already holds a context")
+ {
+ // HONEST LABEL: on an ordinary build the assertions below do NOT detect a
dropped
+ // pcre2_match_context_free(old). Measured: removing that call leaves this
section
+ // passing, because a leak has no observer here. The oracle for the leak is
+ // LeakSanitizer on the ci-rocky lane, which is the only PR check
inheriting asan.
+ //
+ // The section still earns its place: it is what gives LSan an assignment
onto a
+ // live context to observe, which no other case in this file produces.
Without it
+ // the leak is unreachable and therefore undetectable anywhere.
+ RegexMatchContext destination;
+
+ REQUIRE(matches_through(&destination) == expected);
+
+ RegexMatchContext donor;
+
+ destination = std::move(donor);
+
+ REQUIRE(matches_through(&destination) == expected);
+ }
+
+ SECTION("self move assignment")
+ {
+ RegexMatchContext ctx;
+
+ REQUIRE(matches_through(&ctx) == expected);
+
+ // Without the this != &that guard this frees the context and then reads
the freed
+ // pointer back into itself, so the match below runs on freed memory.
+ //
+ // HONEST LABEL: measured, removing that guard leaves this section passing
on an
+ // ordinary build, because a use after free does not reliably fault.
AddressSanitizer
+ // on ci-rocky is the oracle. As above, the value here is producing the
access at all.
+ auto &alias = ctx;
+
+ ctx = std::move(alias);
+
+ REQUIRE(matches_through(&ctx) == expected);
+ }
+}
+
+// The guard from #5762: a pattern that backtracks once per character must fail
+// cleanly rather than run the thread out of stack. PCRE1 recursed on the
machine
+// stack and a long enough subject crashed the server; PCRE2 must report an
error
+// instead.
+TEST_CASE("Regex reports resource exhaustion rather than crashing",
"[libts][Regex][limits]")
+{
+ // Only the JIT path has a bound to exhaust here. PCRE2's interpreter keeps
its
+ // backtracking frames on the heap, so it matches this subject rather than
running
+ // out of anything, and there is no resource error to assert.
+ char const *const pattern =
R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)";
+ if (!pattern_has_jit(pattern)) {
+ SKIP("PCRE2 has no JIT for this pattern, so there is no stack bound to
exhaust");
+ }
+
+ Regex re;
+ REQUIRE(re.compile(pattern));
+
+ // This pattern starts failing at roughly 43KiB of subject against a 1MiB JIT
+ // stack, measured identically on x86_64 and arm64. Smaller JIT frames get
more
+ // subject out of the same stack and so push that threshold up; 256KiB keeps
a
+ // six times margin against that. Do not trim this to just above 43KiB.
+ std::string subject{"/alpha/bravo/?"};
+ subject.append(256 * 1024, 'x');
+
+ RegexMatches matches;
+ int const rc = re.exec(subject, matches);
+ CAPTURE(rc);
+
+ // Reaching this line at all is the crash assertion.
+ REQUIRE(rc < 0);
+ REQUIRE(rc != RE_ERROR_NOMATCH);
+
+ // The paired positive control, and the only part of this case that can tell
a
+ // 1MiB stack from PCRE2's 32KiB fallback: 256KiB overruns both bounds, so
every
+ // assertion above also holds with the JIT stack callback deleted. 10,000 is
seven
+ // times over the fallback's bound and four times under the 1MiB stack's.
Neither
+ // bound pins 1MiB itself; the suite only requires a maximum in roughly
+ // [240KB, 6MB].
+ std::string smaller{"/alpha/bravo/?"};
+ smaller.append(10000, 'x');
+
+ RegexMatches small_matches;
+ int const small_rc = re.exec(smaller, small_matches);
+ CAPTURE(small_rc);
+
+ REQUIRE(small_rc > 0);
+}
+
+namespace
+{
+/** A reusable barrier, so every round starts with all threads alive and
inside it. The
+ * matches of one round then start together on every thread. std::barrier
would say this
+ * directly; it is hand-rolled here to match notstd::barrier in
benchmark_LogObject.cc.
+ *
+ * Each completed round advances the generation, which is what releases the
waiters, so
+ * the barrier can be crossed any number of times. Exactly `expected` threads
must call
+ * arrive_and_wait() every round or the rest block forever, and the barrier
must outlive
+ * all of them, since destroying a condition_variable with waiters on it is
undefined.
+ */
+class RoundBarrier
+{
+public:
+ explicit RoundBarrier(int expected) : _expected{expected} { REQUIRE(expected
> 0); }
+
+ /// Count this thread in, then block until every expected thread has done
the same.
+ void
+ arrive_and_wait()
+ {
+ std::unique_lock<std::mutex> lock{_mutex};
+ auto const generation = _generation;
+
+ if (++_arrived == _expected) {
+ _arrived = 0;
+ ++_generation;
+ _cv.notify_all();
+ return;
+ }
+ _cv.wait(lock, [this, generation]() { return _generation != generation; });
+ }
+
+private:
+ std::mutex _mutex;
+ std::condition_variable _cv;
+ int const _expected;
+ int _arrived = 0;
+ unsigned _generation = 0;
+};
+
+/** A subject the deep pattern below backtracks through once per character.
+ *
+ * At 40 bytes of JIT stack per character the longest of these needs about
223KiB: six
+ * times what PCRE2's 32KiB fallback resolves, and under a quarter of the
shared
+ * context's 1MiB stack. Each thread gets its own length and its own content,
so no two
+ * threads are matching identical bytes.
+ */
+std::string
+stack_hungry_subject(int thread_index)
+{
+ std::string subject(5000 + thread_index * 100, 'a');
+
+ subject[11 + thread_index] = 'b';
+ return subject;
+}
+
+// One pass of the three matches every thread in the concurrency case
performs: a hit that
+// must yield the same three captures, a miss that must stay a miss, and a
deep subject that
+// only resolves on the 1MiB stack. Returns false on the first disagreement,
so a caller
+// counts rounds that went wrong rather than individual assertions. The case
asserts on zero
+// either way, so the change of denominator does not weaken it.
+bool
+concurrent_match_round(Regex const &re_captures, Regex const &re_deep,
std::string const &hit, std::string const &miss,
+ std::string const &deep, RegexMatchContext const *const
use)
+{
+ RegexMatches matches;
+ if (re_captures.exec(hit, matches, 0, use) != 4 || matches[1] != "alpha" ||
matches[2] != "42" || matches[3] != "tail") {
+ return false;
+ }
+
+ RegexMatches no_matches;
+ if (re_captures.exec(miss, no_matches, 0, use) != RE_ERROR_NOMATCH) {
+ return false;
+ }
+
+ // Needs the 1MiB stack, and must consume the whole subject to have used it.
+ RegexMatches deep_matches;
+ if (re_deep.exec(deep, deep_matches, 0, use) <= 0 || deep_matches[0].size()
!= deep.size()) {
+ return false;
+ }
+
+ return true;
+}
+
+} // namespace
+
+// The header promises that exec() may be called concurrently on one instance.
Every thread
+// must reach the same verdict, whether it matches through a caller-supplied
context shared
+// by several threads or through the thread-global one.
+//
+// The deep subjects are longer than PCRE2's 32KiB fallback stack can resolve,
so on a JIT
+// build this also fails if the callback stops handing every thread a 1MiB
stack: deleting
+// the callback, or returning null from it, turns every deep match here into
+// PCRE2_ERROR_JIT_STACKLIMIT. Without a JIT the interpreter resolves them
either way, so
+// only the concurrency half of this case carries over to such a build.
+//
+// Every round starts at a barrier, so all eight threads are alive for the
whole case and
+// each round's matches start together on every thread. The test is
deterministic when the
+// stacks are distinct, but it still does not prove that they are: one stack
shared by all
+// eight corrupts a result only when two deep matches touch the same frames at
once, which
+// the barrier invites but cannot force. Measured on arm64 macOS with the
callback returning
+// one shared stack, about 70 of 100 runs fail, with or without the barrier.
An oracle
+// would need a thread paused inside a match, which the public API does not
offer.
+// ThreadSanitizer does not see those writes either, because they come from
sljit code.
+TEST_CASE("Regex matches concurrently on one instance",
"[libts][Regex][threads]")
+{
+ Regex re_captures;
+ REQUIRE(re_captures.compile(R"(^/([a-z]+)/([0-9]+)/(.*)$)"));
+
+ Regex re_deep;
+ REQUIRE(re_deep.compile(R"(^(?:(a)|(b))+$)"));
+
+ constexpr int THREADS = 8;
+ constexpr int ITERATIONS = 2000;
Review Comment:
Measured on this branch (local arm64 macOS build), the test case takes 0.07
to 0.17 seconds across three runs, so I've left `ITERATIONS` at 2000. The count
matters for what the test can catch: as the comment above it says, a shared
stack corrupts a result only when two deep matches overlap, and fewer rounds
would make that less likely to show up.
--
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]
