From ed645ccf03afb7c1b263e89c4b92ac8d216071d2 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:23:01 -0700 Subject: [PATCH 1/3] Regex: keep the compiled pattern until a recompile succeeds Regex::compile() freed the pattern it already held before calling pcre2_compile(). Every failure path after that point returned with the freed pointer still stored, so empty() reported the object as compiled, exec() passed the freed block to pcre2_match(), and the destructor freed it a second time. Compile into a local and replace the member only after the new pattern exists. A failed compile now leaves the previous pattern in place and usable, which is what a caller checking the return value would expect, and a fresh object that fails to compile is still empty. Two tests cover it: a valid compile followed by a failing one must leave the first pattern matching, including its capture groups. Before this change the first of those segmentation faults. --- include/tsutil/Regex.h | 8 +++++-- src/tsutil/Regex.cc | 13 +++++++----- src/tsutil/unit_tests/test_Regex.cc | 33 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 5b913bece06..1b4291509d2 100644 --- a/include/tsutil/Regex.h +++ b/include/tsutil/Regex.h @@ -163,23 +163,27 @@ class Regex /** Compile the @a pattern into a regular expression. * - * @param pattern Source pattern for regular expression (null terminated). + * @param pattern Source pattern for regular expression. * @param flags Compilation flags. * @return @a true if compiled successfully, @a false otherwise. * * @a flags should be the bitwise @c or of @c REFlags values. + * + * On failure any previously compiled pattern is left in place and remains usable. */ bool compile(std::string_view pattern, uint32_t flags = 0); /** Compile the @a pattern into a regular expression. * - * @param pattern Source pattern for regular expression (null terminated). + * @param pattern Source pattern for regular expression. * @param error String to receive error message. * @param erroffset Pointer to integer to receive error offset. * @param flags Compilation flags. * @return @a true if compiled successfully, @a false otherwise. * * @a flags should be the bitwise @c or of @c REFlags values. + * + * On failure any previously compiled pattern is left in place and remains usable. */ bool compile(std::string_view pattern, std::string &error, int &erroffset, unsigned flags = 0); diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 3281622b1ec..7bea6dae15e 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -394,11 +394,6 @@ Regex::compile(std::string_view pattern, uint32_t flags) bool Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, uint32_t flags) { - // free the existing compiled regex if there is one - if (auto ptr = _Code::get(_code); ptr != nullptr) { - pcre2_code_free(ptr); - } - // get the RegexContext instance - should only be null when shutting down RegexContext *regex_context = RegexContext::get_instance(); if (regex_context == nullptr) { @@ -445,6 +440,14 @@ Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, u // support for JIT pcre2_jit_compile(code, PCRE2_JIT_COMPLETE); + // Replace the previous pattern only now that the new one exists. Freeing it before + // pcre2_compile would leave every failure path above returning with a dangling + // pointer in _code, which empty() reports as a compiled pattern and exec() hands to + // pcre2_match. + if (auto ptr = _Code::get(_code); ptr != nullptr) { + pcre2_code_free(ptr); + } + _Code::set(_code, code); return true; diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index f1bd0a7c866..b7261c0d5d8 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -646,6 +646,39 @@ TEST_CASE("Regex recompilation behavior", "[libts][Regex][recompile]") CHECK(r.exec("valid") == true); } + SECTION("a failed recompile leaves the working pattern in place") + { + // compile() is a transaction. A pattern that fails to compile must not disturb the + // pattern already held, because the alternative is worse than either outcome: freeing + // the old pattern before knowing the new one compiles leaves a dangling pointer that + // empty() reports as compiled and exec() hands to pcre2_match. + Regex r; + REQUIRE(r.compile("foo") == true); + + REQUIRE(r.compile("(invalid") == false); + + CHECK(r.empty() == false); + CHECK(r.exec("foo") == true); + CHECK(r.exec("bar") == false); + + // And the object is still usable for a later successful compile. + REQUIRE(r.compile("bar") == true); + CHECK(r.exec("bar") == true); + } + + SECTION("a failed recompile leaves captures working") + { + Regex r; + REQUIRE(r.compile("^(a+)(b+)$") == true); + + REQUIRE(r.compile("(unterminated") == false); + + RegexMatches matches; + REQUIRE(r.exec("aaabb", matches) == 3); + CHECK(matches[1] == "aaa"); + CHECK(matches[2] == "bb"); + } + SECTION("recompile with different flags") { Regex r; From 473b8ddc2ed55908ab45ec7b2802ca4b37dcf4f6 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 08:41:04 -0700 Subject: [PATCH 2/3] Regex: check compile() in RegexMatcher::NewEntry instead of empty() NewEntry discarded what compile() returned and asked the Regex whether it held a pattern instead. That was already wrong: before this series a failed compile left a freed pointer in the object, so empty() answered false and the bad line was accepted. Making compile() transactional replaces one wrong answer with another, because the object now holds the previous pattern. The slot is genuinely reused. When a line's Data::Init() fails, NewEntry resets regex_strings[num_el] but leaves num_el alone, so the next line lands on the same Regex, which is still carrying the pattern the rejected line compiled. A new line whose own pattern does not compile would then pass the empty() check, get recorded under its own pattern string, and match against the earlier pattern with this line's configuration. Ask compile() whether it compiled. Every other caller in the tree that recompiles a live Regex already does. --- src/proxy/ControlMatcher.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/proxy/ControlMatcher.cc b/src/proxy/ControlMatcher.cc index d24d539988b..44ee14a2b28 100644 --- a/src/proxy/ControlMatcher.cc +++ b/src/proxy/ControlMatcher.cc @@ -424,9 +424,13 @@ RegexMatcher::NewEntry(matcher_line *line_info) ink_assert(line_info->dest_entry < MATCHER_MAX_TOKENS); ink_assert(pattern != nullptr); - // Create the compiled regular expression - regex_array[num_el].compile(pattern, error_msg, erroffset); - if (regex_array[num_el].empty()) { + // Create the compiled regular expression. Check what compile() returned rather than + // asking the object whether it holds a pattern: this slot is reused when a previous + // line's Data::Init() failed, because that path leaves num_el where it was, so the + // Regex here can still hold the pattern that line compiled. empty() would then be + // false for a line whose own pattern never compiled, and the entry would be accepted + // and matched against the earlier pattern under this line's configuration. + if (!regex_array[num_el].compile(pattern, error_msg, erroffset)) { return Result::failure("%s regular expression error at line %d position %d : %s", matcher_name, line_info->line_num, erroffset, error_msg.c_str()); } From b255d770d88fc4b73e4938282b96eca2871438b3 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 11:14:07 -0700 Subject: [PATCH 3/3] Regex: cover the reused RegexMatcher slot, and shorten the comment NewEntry's reuse path had no test. test_ControlMatcher covered UrlMatcher only, so nothing exercised the sequence this change is about: a line that compiles its regex and then fails to initialize its record, followed by a line whose own pattern does not compile landing on the same slot. The new case asserts the second line is rejected, that num_el stays at zero, and that the pattern the first line left behind does not match. Against the old empty() check all three fail, the last one because the stale pattern really does match under the second line's configuration. Also cut the comment at the call site down to the part that is not obvious from the code, which is why this asks compile() rather than the object. The rest of the reasoning is in the previous commit message, where it belongs. --- src/proxy/ControlMatcher.cc | 8 ++---- src/proxy/unit_tests/test_ControlMatcher.cc | 30 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/proxy/ControlMatcher.cc b/src/proxy/ControlMatcher.cc index 44ee14a2b28..4284149d78f 100644 --- a/src/proxy/ControlMatcher.cc +++ b/src/proxy/ControlMatcher.cc @@ -424,12 +424,8 @@ RegexMatcher::NewEntry(matcher_line *line_info) ink_assert(line_info->dest_entry < MATCHER_MAX_TOKENS); ink_assert(pattern != nullptr); - // Create the compiled regular expression. Check what compile() returned rather than - // asking the object whether it holds a pattern: this slot is reused when a previous - // line's Data::Init() failed, because that path leaves num_el where it was, so the - // Regex here can still hold the pattern that line compiled. empty() would then be - // false for a line whose own pattern never compiled, and the entry would be accepted - // and matched against the earlier pattern under this line's configuration. + // Create the compiled regular expression. This slot can still hold a previous line's + // pattern, so ask compile() rather than empty(). if (!regex_array[num_el].compile(pattern, error_msg, erroffset)) { return Result::failure("%s regular expression error at line %d position %d : %s", matcher_name, line_info->line_num, erroffset, error_msg.c_str()); diff --git a/src/proxy/unit_tests/test_ControlMatcher.cc b/src/proxy/unit_tests/test_ControlMatcher.cc index 5fa08804349..c81fc757d25 100644 --- a/src/proxy/unit_tests/test_ControlMatcher.cc +++ b/src/proxy/unit_tests/test_ControlMatcher.cc @@ -116,3 +116,33 @@ TEST_CASE("UrlMatcher rejects duplicate URLs", "[ControlMatcher]") CHECK(second_result.failed()); CHECK(matcher.num_el == 1); } + +TEST_CASE("RegexMatcher rejects a bad pattern at a reused slot", "[ControlMatcher]") +{ + RegexMatcher matcher{"CacheControl", "cache.config"}; + // The first line compiles its regex and then fails to initialize its record, which + // leaves num_el where it was, so the next line lands on the same slot with that + // pattern still in it. + char first_config[] = "url_regex=^http://example.com/good action=invalid"; + char second_config[] = "url_regex=^http://example.com/(unterminated action=never-cache"; + matcher_line first_line; + matcher_line second_line; + + matcher.AllocateSpace(2); + parse_line(first_config, first_line, 1); + parse_line(second_config, second_line, 2); + + CHECK(matcher.NewEntry(&first_line).failed()); + + // The second line's own pattern does not compile, so it must be rejected rather than + // inherit the pattern left in the slot. + CHECK(matcher.NewEntry(&second_line).failed()); + CHECK(matcher.num_el == 0); + + // And the inherited pattern must not match anything. + TestRequestData request{"http://example.com/good"}; + CacheControlResult result; + + matcher.Match(&request, &result); + CHECK_FALSE(result.never_cache); +}