From a69c89869d1b7f1b4592b42d0c1d9ef7ce06630d Mon Sep 17 00:00:00 2001 From: Jered Floyd Date: Tue, 11 Aug 2026 12:07:38 -0400 Subject: [PATCH 1/5] Fix signature on flz_maxcopy to avoid cast error (#13532) flz_maxcopy takes void* arguments but calls fastlz_memcopy (taking uint8_t* arguments) without a cast. I don't see a reason for flz_maxcopy to not take uint8_t* arguments; for arch-specific speedups these get cast to the appropriately-sized pointer. Resolves build issues on ppc64le architecture. (cherry picked from commit ad0b638536030ea98d05359b1f9ae63bd5b680be) --- lib/fastlz/fastlz.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fastlz/fastlz.cc b/lib/fastlz/fastlz.cc index f99bb10b0f3..a012bb94af8 100644 --- a/lib/fastlz/fastlz.cc +++ b/lib/fastlz/fastlz.cc @@ -162,7 +162,7 @@ static void flz_smallcopy(uint8_t* dest, const uint8_t* src, uint32_t count) { } /* special case of memcpy: exactly MAX_COPY bytes */ -static void flz_maxcopy(void* dest, const void* src) { +static void flz_maxcopy(uint8_t* dest, const uint8_t* src) { #if defined(FLZ_ARCH64) const uint32_t* p = (const uint32_t*)src; uint32_t* q = (uint32_t*)dest; From 5e81c3a116904dabcb5b345c77c78319ebb7f32d Mon Sep 17 00:00:00 2001 From: Jered Floyd Date: Tue, 11 Aug 2026 12:08:43 -0400 Subject: [PATCH 2/5] Change libswoc test for undefined errno to a much higher number (#13531) libswoc tests include one that expects errno 134 to be unknown; this is no longer the case in Linux 7.2. This patch increases the errno to one that should be unknown for the forseeable future. (cherry picked from commit a2009d782b3417c21d8b0dbcb3e841f475865bf7) --- lib/swoc/unit_tests/test_bw_format.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/swoc/unit_tests/test_bw_format.cc b/lib/swoc/unit_tests/test_bw_format.cc index caf69a04991..0f5f4a8b07e 100644 --- a/lib/swoc/unit_tests/test_bw_format.cc +++ b/lib/swoc/unit_tests/test_bw_format.cc @@ -525,7 +525,7 @@ TEST_CASE("bwstring std formats", "[libswoc][bwprint]") { w.print("{}", swoc::bwf::Errno(13)); REQUIRE(w.view() == "EACCES: Permission denied [13]"sv); - w.clear().print("{}", swoc::bwf::Errno(134)); + w.clear().print("{}", swoc::bwf::Errno(192)); REQUIRE(w.view().substr(0, 22) == "Unknown: Unknown error"sv); w.clear().print("{:s}", swoc::bwf::Errno(13)); REQUIRE(w.view() == "EACCES: Permission denied"sv); From 51c0e9765089a8e11a9f24d3764df78af646266d Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 11 Aug 2026 13:14:24 -0500 Subject: [PATCH 3/5] Fix an intermittent cache unit test deadlock at exit (#13535) The cache unit test harness starts the event and net processors but never stops them, so every test binary reaches exit() with ET_NET threads still running. Static destruction then frees globals out from under those threads: the records table in RecCore.cc is destroyed while a still-initializing event thread reads it through RecGetRecordInt(), and the ts::Metrics storage blob is released while NetHandler's activity loop increments a counter into it. Both are heap-use-after-frees, and under ASan the reporting thread races the exiting main thread. Usually the process dies first and the report is truncated to two lines with a zero exit status, so ctest reports a pass; occasionally the report deadlocks instead and the test hangs until ctest times it out after 1500 seconds. The short tests that never touch the cache lose this race most often, which is why CacheAggregateWriteBuffer and CacheStripe are the ones that fail. This addresses the deadlock at its source by giving the harness's Catch2 listener a testRunEnded hook that shuts the event system down and joins the event threads before the test binary returns from main. Once the threads are gone, static destruction has no concurrent reader to race, so neither use-after-free can be reported and the ASan reporting deadlock cannot arise. Co-authored-by: Claude Opus 5 (cherry picked from commit 0c18d46bc6fcc311ac94d2ada472f6611ea4a01d) --- src/iocore/cache/unit_tests/main.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/iocore/cache/unit_tests/main.cc b/src/iocore/cache/unit_tests/main.cc index 369b0572b48..1b88e2a380e 100644 --- a/src/iocore/cache/unit_tests/main.cc +++ b/src/iocore/cache/unit_tests/main.cc @@ -162,6 +162,20 @@ struct EventProcessorListener : Catch::EventListenerBase { std::string src_dir = std::string(TS_ABS_TOP_SRCDIR) + "/src/iocore/cache/unit_tests"; Layout::get()->sysconfdir = std::move(src_dir); } + + // Every test binary using this harness reaches exit() with the event threads + // still running, so stop them and wait for them before static destruction + // frees the globals they read. + void + testRunEnded(Catch::TestRunStats const & /* stats ATS_UNUSED */) override + { + TSSystemState::shut_down_event_system(); + for (EThread *ethread : eventProcessor.active_ethreads()) { + if (ethread->tid != ink_thread_null()) { + ink_thread_join(ethread->tid); + } + } + } }; CATCH_REGISTER_LISTENER(EventProcessorListener); From 21f5363e27a287aa32623a3b3a082ccad78c2f27 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 11 Aug 2026 21:11:15 -0500 Subject: [PATCH 4/5] cache: fix shm sizing on large-page Linux (#13536) POSIX shared-memory objects on Linux retain the exact length passed to ftruncate(), but the cache shm gates accepted any size through the next page boundary. On 64 KiB-page systems, a foreign control layout could therefore be treated as compatible, causing cleanup and tooling paths to walk an untrusted stripe table and leave segments behind. This patch requires exact shared-memory object sizes outside macOS while preserving macOS's page-rounded allowance. It keeps the foreign-layout test at its original size and directly covers the platform-specific sizing contract. Fixes: #13534 (cherry picked from commit f0668e112d79e55a9b9dbb4192c85c6a5e871f2c) --- include/shared/cache_shm/Layout.h | 21 ++++++++++++++++---- include/shared/cache_shm/Purge.h | 7 +++---- src/iocore/cache/CacheShm.cc | 8 ++------ src/iocore/cache/unit_tests/test_CacheShm.cc | 17 ++++++++++++++-- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/include/shared/cache_shm/Layout.h b/include/shared/cache_shm/Layout.h index fcaaa11280e..4255af5706c 100644 --- a/include/shared/cache_shm/Layout.h +++ b/include/shared/cache_shm/Layout.h @@ -87,13 +87,26 @@ constexpr std::size_t CONTROL_HEADER_SIZE = offsetof(CacheShmControl, stripes); static_assert(CONTROL_HEADER_SIZE == 48, "the control segment header is a frozen layout; see the comment above"); static_assert(std::is_standard_layout_v, "the control segment is shared across processes and builds"); -// Whether a control segment of `actual` bytes was written by *this* build; the kernel rounds an shm object up to a page. -// Anything larger has a stripes[] of unknown stride and must never be walked with our layout. Shared by the attach gate, -// the purge primitive and `traffic_ctl cache shm status` so the three cannot drift apart. +/// Whether @a actual is the object size this platform reports after truncating +/// a POSIX shared-memory object to @a requested bytes. +inline bool +is_expected_shm_size(std::size_t actual, std::size_t requested) +{ +#if defined(__APPLE__) + // macOS rounds the reported object size up to the VM page size. + return actual >= requested && actual <= INK_ALIGN(requested, ats_pagesize()); +#else + return actual == requested; +#endif +} + +// Whether a control segment of `actual` bytes was written by *this* build. Anything else has a stripes[] of unknown +// stride and must never be walked with our layout. Shared by the attach gate, the purge primitive and +// `traffic_ctl cache shm status` so the three cannot drift apart. inline bool is_own_control_size(std::size_t actual) { - return actual >= CONTROL_SIZE && actual <= INK_ALIGN(CONTROL_SIZE, ats_pagesize()); + return is_expected_shm_size(actual, CONTROL_SIZE); } // Frame the operator's middle word (e.g. "ats") as "/-". The framing is supplied here so it cannot be mis-typed: diff --git a/include/shared/cache_shm/Purge.h b/include/shared/cache_shm/Purge.h index 1b170b385e1..39ff9966bcb 100644 --- a/include/shared/cache_shm/Purge.h +++ b/include/shared/cache_shm/Purge.h @@ -286,10 +286,9 @@ purge_segments(const std::string &prefix) return report; } - // Larger than this build's page-rounded CONTROL_SIZE means a build with a different - // sizeof(CacheShmControl) wrote it. The frozen header prefix is still readable (so - // the owner guard above applies), but stripes[] may have a different stride entirely, - // so its names must not drive shm_unlink. + // A control size this build does not accept means a build with a different sizeof(CacheShmControl) wrote it. The frozen + // header prefix is still readable (so the owner guard above applies), but stripes[] may have a different stride + // entirely, so its names must not drive shm_unlink. if (magic_ok && is_own_control_size(static_cast(sb.st_size))) { unlink_table_stripes(prefix, ctrl, report.unlinked); } else { diff --git a/src/iocore/cache/CacheShm.cc b/src/iocore/cache/CacheShm.cc index 157a5097b54..69569822b79 100644 --- a/src/iocore/cache/CacheShm.cc +++ b/src/iocore/cache/CacheShm.cc @@ -270,14 +270,10 @@ open_and_map_shm(const std::string &name, std::size_t size, ShmAccess access, [[ return nullptr; } } else { - // The kernel rounds an shm object up to a page, so accept any size in [requested, page-up]. struct stat sb { }; - std::size_t expected_max = INK_ALIGN(size, ats_pagesize()); - if (fstat(fd, &sb) < 0 || sb.st_size < 0 || static_cast(sb.st_size) < size || - static_cast(sb.st_size) > expected_max) { - Dbg(dbg_ctl, "shm %s size mismatch (have %lld, want %zu, max %zu)", name.c_str(), static_cast(sb.st_size), size, - expected_max); + if (fstat(fd, &sb) < 0 || sb.st_size < 0 || !cache_shm::is_expected_shm_size(static_cast(sb.st_size), size)) { + Dbg(dbg_ctl, "shm %s size mismatch (have %lld, want %zu)", name.c_str(), static_cast(sb.st_size), size); return nullptr; } } diff --git a/src/iocore/cache/unit_tests/test_CacheShm.cc b/src/iocore/cache/unit_tests/test_CacheShm.cc index 790e4841845..87380133bf0 100644 --- a/src/iocore/cache/unit_tests/test_CacheShm.cc +++ b/src/iocore/cache/unit_tests/test_CacheShm.cc @@ -225,6 +225,19 @@ TEST_CASE("CacheShm process liveness check backs the concurrent-attach guard", " CHECK_FALSE(CacheShm::process_is_alive(std::numeric_limits::max())); } +TEST_CASE("CacheShm object size matching follows platform behavior", "[cache][shm]") +{ + constexpr std::size_t requested = cache_shm::CONTROL_SIZE; + + CHECK(cache_shm::is_expected_shm_size(requested, requested)); + CHECK_FALSE(cache_shm::is_expected_shm_size(requested - 1, requested)); +#if defined(__APPLE__) + CHECK(cache_shm::is_expected_shm_size(INK_ALIGN(requested, ats_pagesize()), requested)); +#else + CHECK_FALSE(cache_shm::is_expected_shm_size(requested + sizeof(cache_shm::StripeEntry), requested)); +#endif +} + // The rest of this file needs real shm objects, unlike the layout/fingerprint cases // above, so it is gated the same way the feature is. #if TS_USE_CACHE_SHM @@ -293,8 +306,8 @@ segment_exists(const std::string &name) return true; } -// The kernel rounds an shm object up to a page, so a segment shorter than CONTROL_SIZE is not representable everywhere: -// Apple Silicon's 16 KB page already exceeds it. -1 if the segment is gone. +// macOS rounds a POSIX shm object up to a page, so a segment shorter than CONTROL_SIZE is not representable there. +// -1 if the segment is gone. long long segment_size(const std::string &name) { From 3ac694d766844524a5bc49a6671f96092ca7fb27 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 11 Aug 2026 22:21:10 -0500 Subject: [PATCH 5/5] Share regex_remap rule sets (#13537) Reloading remap.config becomes slow when it contains many mappings that reference the same regex_remap rule files. Since the PCRE2 conversion, every plugin instance JIT-compiles an independent copy, making reload time scale with instances rather than unique rule sets. This patch caches immutable compiled rule sets by resolved filename and exact source content. It keeps match contexts and profiling counters per instance and uses weak ownership so obsolete reload generations are released, preserving JIT request performance without redundant reload work. (cherry picked from commit f869b9c1adac59fad6ba81a29a4c73ca8bb3a228) --- plugins/regex_remap/regex_remap.cc | 506 +++++++++++------- .../regex_remap/regex_remap.test.py | 140 ++++- 2 files changed, 427 insertions(+), 219 deletions(-) diff --git a/plugins/regex_remap/regex_remap.cc b/plugins/regex_remap/regex_remap.cc index 9c5d6e3834d..86b2b2d2ba7 100644 --- a/plugins/regex_remap/regex_remap.cc +++ b/plugins/regex_remap/regex_remap.cc @@ -25,6 +25,7 @@ #include "ts/remap_version.h" #include +#include #include #include #include @@ -32,12 +33,16 @@ #include #include +#include #include #include #include #include #include +#include #include +#include +#include // Get some specific stuff from libts, yes, we can do that now that we build inside the core. #include "tscore/ink_platform.h" @@ -104,30 +109,49 @@ struct UrlComponents { }; /////////////////////////////////////////////////////////////////////////////// -// Class encapsulating one regular expression (and the linked list). +// One immutable remap rule: the compiled regex plus its substitution template +// and per-rule options (status, timeouts, strategy, config overrides). +// +// A RemapRegex is shared. Once compile_rule_set() hands it to RuleSet::add() it +// is const, and it is read concurrently and without locks by every ET_NET +// thread of every RemapInstance that loaded the same rule file. Nothing here +// may be mutated after that point, and nothing per-instance or per-transaction +// may be stored here. That is why the match context and profiling hit counts +// are passed in as arguments rather than kept as members: they belong to +// RemapInstance. Put new per-instance state on RemapInstance, indexed in +// lockstep with RuleSet::rules(), never on this class. // class RemapRegex { public: + RemapRegex() = default; + RemapRegex(RemapRegex const &) = delete; + RemapRegex &operator=(RemapRegex const &) = delete; + ~RemapRegex() { Dbg(dbg_ctl, "Calling destructor"); TSfree(_rex_string); TSfree(_subst); + + while (_first_override) { + Override *tmp = _first_override; + + _first_override = _first_override->next; + if (TS_RECORDDATATYPE_STRING == tmp->type) { + TSfree(tmp->data.rec_string); + } + delete tmp; + } } bool initialize(const std::string ®, const std::string &sub, const std::string &opt); - // For profiling information - void - increment() - { - ink_atomic_increment(&(_hits), 1); - } + // Profiling output for one rule, as a percentage of this instance's matches. void - print(int ix, int max, const char *now) + print(int ix, int total_hits, int rule_hits, const char *now) const { - fprintf(stderr, "[%s]: Regex %d ( %s ): %.2f%%\n", now, ix, _rex_string, 100.0 * _hits / max); + fprintf(stderr, "[%s]: Regex %d ( %s ): %.2f%%\n", now, ix, _rex_string, 100.0 * rule_hits / total_hits); } // Returns '0' on success @@ -135,10 +159,9 @@ class RemapRegex // number of matches, or negative if failed int - match(std::string_view const str, RegexMatches &matches) const + match(std::string_view const str, RegexMatches &matches, RegexMatchContext const *match_context) const { - TSAssert(nullptr != _match_context); - int const stat = _rex.exec(str, matches, 0, _match_context); + int const stat = _rex.exec(str, matches, 0, match_context); if (0 <= stat) { Dbg(dbg_ctl, "Regex match (%d): %.*s", stat, (int)str.length(), str.data()); return matches.size(); @@ -147,39 +170,9 @@ class RemapRegex } // Substitutions - int get_lengths(RegexMatches const &matches, int lengths[], TSRemapRequestInfo *rri, UrlComponents *req_url); + int get_lengths(RegexMatches const &matches, int lengths[], TSRemapRequestInfo *rri, UrlComponents *req_url) const; int substitute(char dest[], RegexMatches const &matches, const int lengths[], TSHttpTxn txnp, TSRemapRequestInfo *rri, - UrlComponents *req_url, bool lowercase_substitutions); - - // setter / getters for members the linked list. - inline void - set_next(RemapRegex *next) - { - _next = next; - } - inline RemapRegex * - next() const - { - return _next; - } - - inline void - set_match_context(RegexMatchContext const *const ctx) - { - _match_context = ctx; - } - - // setter / getters for order number within the linked list - inline void - set_order(int order) - { - _order = order; - } - inline int - order() - { - return _order; - } + UrlComponents *req_url, bool lowercase_substitutions) const; // Various getters inline const char * @@ -242,27 +235,22 @@ class RemapRegex Override *next; }; - Override * + Override const * get_overrides() const { return _first_override; } private: - char *_rex_string = nullptr; - char *_subst = nullptr; - int _subst_len = 0; - int _num_subs = -1; - int _hits = 0; - int _options = 0; - int _order = -1; + char *_rex_string = nullptr; + char *_subst = nullptr; + int _subst_len = 0; + int _num_subs = -1; + int _options = 0; + bool _lowercase_substitutions = false; - bool _lowercase_substitutions = false; - - Regex _rex; - RegexMatchContext const *_match_context = nullptr; // owned by RemapInstance - RemapRegex *_next = nullptr; - TSHttpStatus _status = static_cast(0); + Regex _rex; + TSHttpStatus _status = static_cast(0); int _active_timeout = -1; int _no_activity_timeout = -1; @@ -468,7 +456,7 @@ RemapRegex::compile(std::string &error, int &erroffset) // We also calculate a total length for the new string, which is the max length the // substituted string can have (used for the ts::LocalBuffer). int -RemapRegex::get_lengths(RegexMatches const &matches, int lengths[], TSRemapRequestInfo *rri, UrlComponents *req_url) +RemapRegex::get_lengths(RegexMatches const &matches, int lengths[], TSRemapRequestInfo *rri, UrlComponents *req_url) const { int len = _subst_len + 1; // Bigger then necessary @@ -523,7 +511,7 @@ RemapRegex::get_lengths(RegexMatches const &matches, int lengths[], TSRemapReque // length of the string as written to dest (not including the trailing '0'). int RemapRegex::substitute(char dest[], RegexMatches const &matches, const int lengths[], TSHttpTxn txnp, TSRemapRequestInfo *rri, - UrlComponents *req_url, bool lowercase_substitutions) + UrlComponents *req_url, bool lowercase_substitutions) const { if (_num_subs > 0) { char *p1 = dest; @@ -607,12 +595,219 @@ RemapRegex::substitute(char dest[], RegexMatches const &matches, const int lengt return 0; // Shouldn't happen. } +/////////////////////////////////////////////////////////////////////////////// +// One immutable generation of a regex_remap rule file. The exact source bytes +// are retained so a reload can distinguish changed content while the previous +// generation remains live. +// +class RuleSet +{ +public: + using Rules = std::vector>; + + explicit RuleSet(std::string const &source) : _source(source) {} + + bool + has_source(std::string const &source) const + { + // Remap reloads build the new table before releasing the old one. Compare + // exact bytes so a still-live generation for this filename cannot give new + // instances stale rules after the file changes. + return _source == source; + } + + void + add(std::unique_ptr rule) + { + _rules.push_back(std::move(rule)); + } + + Rules const & + rules() const + { + return _rules; + } + +private: + std::string _source; + Rules _rules; +}; + +using SharedRuleSet = std::shared_ptr; + +/////////////////////////////////////////////////////////////////////////////// +// Compile and publish one immutable rule-file generation. +// +SharedRuleSet +compile_rule_set(std::string const &filename, std::string const &source) +{ + auto rule_set = std::make_shared(source); + std::istringstream input(source); + int lineno = 0; + std::string line; + + while (getline(input, line)) { + std::string regex, subst, options; + std::string::size_type pos1, pos2; + + ++lineno; + if (line.empty()) { + continue; + } + + pos1 = line.find_first_not_of(" \t\n"); + if (pos1 != std::string::npos) { + if (line[pos1] == '#') { + continue; + } + + pos2 = line.find_first_of(" \t\n", pos1); + if (pos2 != std::string::npos) { + regex = line.substr(pos1, pos2 - pos1); + pos1 = line.find_first_not_of(" \t\n#", pos2); + if (pos1 != std::string::npos) { + pos2 = line.find_first_of(" \t\n", pos1); + if (pos2 == std::string::npos) { + pos2 = line.length(); + } + subst = line.substr(pos1, pos2 - pos1); + pos1 = line.find_first_not_of(" \t\n#", pos2); + if (pos1 != std::string::npos) { + pos2 = line.find_first_of("\n#", pos1); + if (pos2 == std::string::npos) { + pos2 = line.length(); + } + options = line.substr(pos1, pos2 - pos1); + } + } + } + } + + if (regex.empty()) { + TSError("[%s] no regexp found in %s: line %d", PLUGIN_NAME, filename.c_str(), lineno); + continue; + } + if (subst.empty() && options.empty()) { + TSError("[%s] no substitution string found in %s: line %d", PLUGIN_NAME, filename.c_str(), lineno); + continue; + } + + auto cur = std::make_unique(); + + if (!cur->initialize(regex, subst, options)) { + TSError("[%s] can't create a new regex remap rule", PLUGIN_NAME); + continue; + } + + std::string error; + int erroffset; + Dbg(dbg_ctl, "Compiling regex: %s", regex.c_str()); + if (0 != cur->compile(error, erroffset)) { + std::ostringstream oss; + oss << '[' << PLUGIN_NAME << "] Regex compile failed in " << filename << " (line " << lineno << ')'; + if (erroffset > 0) { + oss << " at offset " << erroffset; + } + oss << ": " << error; + if (cur->regex_empty()) { + oss << " (no regular expression)"; + } else { + oss << " regex: \"" << cur->regex() << '"'; + } + TSError("%s", oss.str().c_str()); + continue; + } + + Dbg(dbg_ctl, "Added regex=%s with subs=%s and options `%s'", regex.c_str(), subst.c_str(), options.c_str()); + rule_set->add(std::move(cur)); + } + + if (rule_set->rules().empty()) { + TSError("[%s] no regular expressions from the maps", PLUGIN_NAME); + return nullptr; + } + + return rule_set; +} + +class RuleSetCache +{ + /// Live generations of one rule file. More than one entry exists only while + /// an old config generation is still draining and the file content changed. + /// Weak ownership is deliberate: the cache must never keep a RuleSet alive. + using Generations = std::vector>; + using Entries = std::unordered_map; + +public: + static SharedRuleSet + get(std::string const &filename, std::string const &source) + { + std::lock_guard lock(mutex()); + auto &entries = cache(); + + auto existing = entries.find(filename); + if (existing != entries.end()) { + std::erase_if(existing->second, [](std::weak_ptr const &entry) { return entry.expired(); }); + for (auto const &entry : existing->second) { + if (auto rule_set = entry.lock(); rule_set && rule_set->has_source(source)) { + Dbg(dbg_ctl, "Reusing cached regular expressions from %s", filename.c_str()); + return rule_set; + } + } + if (existing->second.empty()) { + entries.erase(existing); + } + } + + // Pruning is bookkeeping, not correctness: lock() above cannot return an + // expired generation. Keep the full sweep off the once-per-mapping hit path. + prune_expired(entries); + + // Compile with the lock held. Concurrent instances of the same source must + // serialize here, or each would compile a copy and all but one would be wasted. + auto rule_set = compile_rule_set(filename, source); + if (rule_set) { + entries[filename].push_back(rule_set); + Dbg(dbg_ctl, "Cached regular expressions from %s", filename.c_str()); + } + return rule_set; + } + +private: + static void + prune_expired(Entries &entries) + { + for (auto file = entries.begin(); file != entries.end();) { + std::erase_if(file->second, [](std::weak_ptr const &entry) { return entry.expired(); }); + if (file->second.empty()) { + file = entries.erase(file); + } else { + ++file; + } + } + } + + static Entries & + cache() + { + static Entries entries; + return entries; + } + + static std::mutex & + mutex() + { + static std::mutex lock; + return lock; + } +}; + // Hold one remap instance struct RemapInstance { RemapInstance() : filename("unknown") {} - RemapRegex *first = nullptr; - RemapRegex *last = nullptr; + SharedRuleSet rule_set; + std::vector rule_hits; RegexMatchContext match_context = {}; bool pristine_url = false; bool profile = false; @@ -644,10 +839,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE { RemapInstance *ri = new RemapInstance(); - std::ifstream f; - int lineno = 0; - int count = 0; - *ih = (void *)ri; if (ri == nullptr) { TSError("[%s] Unable to create remap instance", PLUGIN_NAME); @@ -696,112 +887,40 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE ri->filename += argv[2]; } - if (0 != access(ri->filename.c_str(), R_OK)) { - TSError("[%s] failed to access %s: %s", PLUGIN_NAME, ri->filename.c_str(), strerror(errno)); + struct stat st; + if (0 != stat(ri->filename.c_str(), &st)) { + TSError("[%s] failed to stat %s: %s", PLUGIN_NAME, ri->filename.c_str(), strerror(errno)); + return TS_ERROR; + } + if (!S_ISREG(st.st_mode)) { + TSError("[%s] %s is not a regular file", PLUGIN_NAME, ri->filename.c_str()); return TS_ERROR; } - f.open((ri->filename).c_str(), std::ios::in); + std::ifstream f((ri->filename).c_str(), std::ios::in | std::ios::binary); if (!f.is_open()) { TSError("[%s] unable to open %s", PLUGIN_NAME, (ri->filename).c_str()); return TS_ERROR; } Dbg(dbg_ctl, "Loading regular expressions from %s", (ri->filename).c_str()); - while (!f.eof()) { - std::string line, regex, subst, options; - std::string::size_type pos1, pos2; - - getline(f, line); - ++lineno; - if (line.empty()) { - continue; - } - - pos1 = line.find_first_not_of(" \t\n"); - if (pos1 != std::string::npos) { - if (line[pos1] == '#') { - continue; // Skip comment lines - } - - pos2 = line.find_first_of(" \t\n", pos1); - if (pos2 != std::string::npos) { - regex = line.substr(pos1, pos2 - pos1); - pos1 = line.find_first_not_of(" \t\n#", pos2); - if (pos1 != std::string::npos) { - pos2 = line.find_first_of(" \t\n", pos1); - if (pos2 == std::string::npos) { - pos2 = line.length(); - } - subst = line.substr(pos1, pos2 - pos1); - pos1 = line.find_first_not_of(" \t\n#", pos2); - if (pos1 != std::string::npos) { - pos2 = line.find_first_of("\n#", pos1); - if (pos2 == std::string::npos) { - pos2 = line.length(); - } - options = line.substr(pos1, pos2 - pos1); - } - } - } - } - - if (regex.empty()) { - // No regex found on this line - TSError("[%s] no regexp found in %s: line %d", PLUGIN_NAME, (ri->filename).c_str(), lineno); - continue; - } - if (subst.empty() && options.empty()) { - // No substitution found on this line (and no options) - TSError("[%s] no substitution string found in %s: line %d", PLUGIN_NAME, (ri->filename).c_str(), lineno); - continue; - } - - // Got a regex and substitution string - std::unique_ptr cur(new RemapRegex); - - if (!cur->initialize(regex, subst, options)) { - TSError("[%s] can't create a new regex remap rule", PLUGIN_NAME); - continue; - } - - std::string error; - int erroffset; - Dbg(dbg_ctl, "Compiling regex: %s", regex.c_str()); - if (0 != cur->compile(error, erroffset)) { - std::ostringstream oss; - oss << '[' << PLUGIN_NAME << "] Regex compile failed in " << (ri->filename).c_str() << " (line " << lineno << ')'; - if (erroffset > 0) { - oss << " at offset " << erroffset; - } - oss << ": " << error; - if (cur->regex_empty()) { - oss << " (no regular expression)"; - } else { - oss << " regex: \"" << cur->regex() << '"'; - } - TSError("%s", oss.str().c_str()); - } else { - Dbg(dbg_ctl, "Added regex=%s with subs=%s and options `%s'", regex.c_str(), subst.c_str(), options.c_str()); - cur->set_order(++count); - cur->set_match_context(&(ri->match_context)); - auto tmp = cur.get(); - if (ri->first == nullptr) { - ri->first = cur.release(); - } else { - ri->last->set_next(cur.release()); - } - ri->last = tmp; - } + std::streamsize const expected_size = st.st_size; + std::string source(static_cast(expected_size), '\0'); + f.read(source.data(), expected_size); + if (f.bad() || f.gcount() != expected_size) { + TSError("[%s] short read on %s: got %lld of %lld bytes", PLUGIN_NAME, ri->filename.c_str(), static_cast(f.gcount()), + static_cast(expected_size)); + return TS_ERROR; } - ri->match_context.set_match_limit(REGEX_MATCH_LIMIT); - - // Make sure we got something... - if (ri->first == nullptr) { - TSError("[%s] no regular expressions from the maps", PLUGIN_NAME); + ri->rule_set = RuleSetCache::get(ri->filename, source); + if (!ri->rule_set) { return TS_ERROR; } + ri->match_context.set_match_limit(REGEX_MATCH_LIMIT); + if (ri->profile) { + ri->rule_hits.resize(ri->rule_set->rules().size()); + } return TS_SUCCESS; } @@ -811,8 +930,6 @@ TSRemapDeleteInstance(void *ih) { Dbg(dbg_ctl, "TSRemapDeleteInstance"); RemapInstance *ri = static_cast(ih); - RemapRegex *re; - RemapRegex *tmp; if (ri->profile) { char now[64]; @@ -830,33 +947,11 @@ TSRemapDeleteInstance(void *ih) fprintf(stderr, "[%s]: Total regex internal errors: %d\n", now, ri->failures); if (ri->hits > 0) { // Avoid divide by zeros... - int ix = 1; - - re = ri->first; - while (re) { - re->print(ix, ri->hits, now); - re = re->next(); - ++ix; - } - } - } - - re = ri->first; - while (re) { - RemapRegex::Override *override = re->get_overrides(); - - while (override) { - RemapRegex::Override *tmp = override; - - if (TS_RECORDDATATYPE_STRING == override->type) { - TSfree(override->data.rec_string); + auto const &rules = ri->rule_set->rules(); + for (size_t ix = 0; ix < rules.size(); ++ix) { + rules[ix]->print(ix + 1, ri->hits, ri->rule_hits[ix], now); } - override = override->next; - delete tmp; } - tmp = re; - re = re->next(); - delete tmp; } delete ri; @@ -873,6 +968,10 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) return TSREMAP_NO_REMAP; } RemapInstance *ri = static_cast(ih); + if (!ri->rule_set) { + Dbg(dbg_ctl, "No rule set on this instance, skipping"); + return TSREMAP_NO_REMAP; + } struct SrcUrl { TSMBuffer bufp; @@ -904,8 +1003,7 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) int lengths[MATCHCOUNT + 1]; int dest_len; - TSRemapStatus retval = TSREMAP_DID_REMAP; - RemapRegex *re = ri->first; + TSRemapStatus retval = TSREMAP_NO_REMAP; int match_len = 0; // Cap the stack allocation to 16KB, a typical browser upper limit @@ -951,10 +1049,17 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) RegexMatches matches(MATCHCOUNT); - // Apply the regular expressions, in order. First one wins. - while (re) { + auto const &rules = ri->rule_set->rules(); + + // Apply the rules in file order; the first one that matches wins. This relies + // on get_lengths() always returning a positive length for a matching rule. If + // that changes, a match could fall through after applying its options and let + // a later rule stack its options on top. + for (size_t rule_ix = 0; rule_ix < rules.size(); ++rule_ix) { + auto const &re = rules[rule_ix]; + // Since we check substitutions on parse time, we don't need to reset ovector - auto match_result = re->match(match_buf.data(), matches); + auto match_result = re->match(match_buf.data(), matches, &(ri->match_context)); if (match_result >= 0) { int new_len = re->get_lengths(matches, lengths, rri, &req_url); @@ -996,7 +1101,7 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) lowercase_substitutions = true; } - RemapRegex::Override *override = re->get_overrides(); + RemapRegex::Override const *override = re->get_overrides(); while (override) { switch (override->type) { @@ -1020,11 +1125,13 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) // Update profiling if requested if (ri->profile) { - re->increment(); + ink_atomic_increment(&(ri->rule_hits[rule_ix]), 1); ink_atomic_increment(&(ri->hits), 1); } if (new_len > 0) { + retval = TSREMAP_DID_REMAP; + // Cap the stack allocation to 16KB, a typical browser upper limit ts::LocalBuffer dest(new_len + 8); @@ -1032,7 +1139,7 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) Dbg(dbg_ctl, "New URL is estimated to be %d bytes long, or less", new_len); Dbg(dbg_ctl, "New URL is %s (length %d)", dest.data(), dest_len); - Dbg(dbg_ctl, " matched rule %d [%s]", re->order(), re->regex()); + Dbg(dbg_ctl, " matched rule %zu [%s]", rule_ix + 1, re->regex()); // Check for a quick response, if the status option is set if (re->status_option() > 0) { @@ -1067,15 +1174,10 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri) TSError(R"([%s] Bad regular expression result %d ("%s") from "%s" in file "%s".)", PLUGIN_NAME, match_result, errmsg.c_str(), re->regex(), ri->filename.c_str()); } + } - // Try the next regex - re = re->next(); - if (re == nullptr) { - retval = TSREMAP_NO_REMAP; // No match - if (ri->profile) { - ink_atomic_increment(&(ri->misses), 1); - } - } + if (retval == TSREMAP_NO_REMAP && ri->profile) { + ink_atomic_increment(&(ri->misses), 1); } return retval; diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py index 1c98e109090..91d4cb7ab3a 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -15,7 +15,6 @@ # limitations under the License. import os -import time import json Test.Summary = ''' @@ -23,14 +22,11 @@ ''' # Test description: -# Load up cache, ensure fresh -# Create regex reval rule, config reload: -# ensure item is staled only once. -# Add a new rule, config reload: -# ensure item isn't restaled again, but rule still in effect. -# -# If the rule disappears from regex_revalidate.conf its still loaded!! -# A rule's expiry can't be changed after the fact! +# Exercise regex_remap rule matching, redirects, pristine-URL mapping, and the +# regex match limit. Then verify that two map rules naming the same rule file +# share one compiled rule set, and that rewriting that file and reloading +# remap.config compiles a new shared generation instead of reusing the live one. +# No rule here uses a $n / $h substitution, so that path is not covered. Test.SkipUnless(Condition.PluginExists('regex_remap.so'),) Test.ContinueOnFail = False @@ -55,13 +51,13 @@ regex_remap2_conf_path = os.path.join(ts.Variables.CONFIGDIR, 'regex_remap2.conf') curl_and_args = '-s -D - -v --proxy localhost:{} '.format(ts.Variables.port) -ts.Disk.File( - regex_remap_conf_path, typename="ats:config").AddLines( - [ - "# regex_remap configuration\n" - "^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$ https://redirect.com/ @status=301\n" - "^/match_limit/(a+)+$ https://redirect.com/ @status=301\n" - ]) +regex_remap_lines = [ + "# regex_remap configuration\n", + "^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$ https://redirect.com/ @status=301\n", + "^/match_limit/(a+)+$ https://redirect.com/ @status=301\n", +] + +ts.Disk.File(regex_remap_conf_path, typename="ats:config").AddLines(regex_remap_lines) ts.Disk.File( regex_remap2_conf_path, typename="ats:config").AddLines( @@ -79,7 +75,7 @@ "map http://example.three/ http://wrong.com/ ".format(server.Variables.Port) + "@plugin=regex_remap.so @pparam=regex_remap2.conf @pparam=pristine\n") -# minimal configuration +# The cache assertions below depend on regex_remap remaining in the debug tags. ts.Disk.records_config.update( { 'proxy.config.diags.debug.enabled': 1, @@ -141,3 +137,113 @@ ts.Disk.diags_log.Content = Testers.ContainsExpression( 'ERROR: .regex_remap. Bad regular expression result -47', "Match limit exceeded") tr.StillRunningAfter = ts + + +class TestRegexRemapRuleCache: + '''Verify shared compiled rules across a remap.config reload.''' + + updated_rule = "^/cache-generation$ https://updated.example/ @status=302\n" + + def __init__(self, ts_process: 'Process', original_rules: str, curl_args: str): + '''Configure the cache and reload TestRuns.''' + self._ts = ts_process + self._original_rules = original_rules + self._curl_args = curl_args + self._regex_remap_path = os.path.join(ts_process.Variables.CONFIGDIR, 'regex_remap.conf') + self._remap_path = os.path.join(ts_process.Variables.CONFIGDIR, 'remap.config') + + self._add_rule_update_run() + self._add_reload_run() + self._add_new_generation_run() + self._add_shared_generation_run() + self._add_isolated_generation_run() + self._add_cache_verification_run() + + def _update_rules(self) -> None: + '''Write a new rule generation and mark remap.config as changed.''' + with open(self._regex_remap_path, 'w') as config_file: + config_file.write(self.updated_rule + self._original_rules) + os.utime(self._remap_path) + + def _add_rule_update_run(self) -> 'TestRun': + '''Change the shared rule file while its first generation is live.''' + tr = Test.AddTestRun("change shared regex_remap rules") + tr.Processes.Default.Command = "echo 'Updating shared regex_remap rules'" + tr.Processes.Default.Setup.Lambda(self._update_rules) + tr.Processes.Default.ReturnCode = 0 + tr.StillRunningAfter = self._ts + return tr + + def _add_reload_run(self) -> 'TestRun': + '''Reload remap.config after the shared rule file changes.''' + tr = Test.AddConfigReload(self._ts, expect_tasks=["remap.config"], description="Reload changed shared regex_remap rules") + tr.StillRunningAfter = self._ts + return tr + + def _add_new_generation_run(self) -> 'TestRun': + '''Verify the new rule generation is active after the reload.''' + tr = Test.AddTestRun("new shared rule generation") + tr.MakeCurlCommand(self._curl_args + "'http://example.one/cache-generation' | grep -e '^HTTP/' -e '^Location'", ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression("HTTP/1.1 302", "New rule returns a redirect") + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "Location: https://updated.example/", "New rule generation is active") + tr.StillRunningAfter = self._ts + return tr + + def _add_shared_generation_run(self) -> 'TestRun': + '''Verify the other mapping on this file sees the same generation.''' + tr = Test.AddTestRun("second mapping sees same rule generation") + tr.MakeCurlCommand(self._curl_args + "'http://example.two/cache-generation' | grep -e '^HTTP/' -e '^Location'", ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression("HTTP/1.1 302", "Sharing mapping returns a redirect") + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "Location: https://updated.example/", "Sharing mapping is on the new generation") + tr.StillRunningAfter = self._ts + return tr + + def _add_isolated_generation_run(self) -> 'TestRun': + '''Verify a different rule file does not reuse the changed generation.''' + tr = Test.AddTestRun("different rule file remains isolated") + tr.MakeCurlCommand( + self._curl_args + "'http://example.three/cache-generation' | grep -e '^HTTP/' -e '^Location'", ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression("HTTP/1.1 302", "Different file does not redirect") + tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression( + "Location: https://updated.example/", "Different file does not use the changed generation") + tr.StillRunningAfter = self._ts + return tr + + def _add_cache_verification_run(self) -> 'TestRun': + '''Verify each distinct generation is compiled only once.''' + await_tr = Test.AddAwaitFileContainsTestRun( + "await rule cache debug output", self._ts.Disk.traffic_out.Name, "Reusing cached regular expressions from", 3) + await_tr.StillRunningAfter = self._ts + + # Compiles: regex_remap.conf gen1, regex_remap2.conf gen1, and + # regex_remap.conf gen2 == 3 generations and 6 regular expressions. + # Reuses: example.two shares regex_remap.conf on both loads, while + # example.three reuses regex_remap2.conf across the build-then-swap + # reload because the previous remap table is still holding it == 3. + tr = Test.AddTestRun("verify compiled rule cache") + tr.Processes.Default.Command = ( + f"log={self._ts.Disk.traffic_out.Name}; " + "cached=$$(grep -c 'Cached regular expressions from' $$log); " + "reused=$$(grep -c 'Reusing cached regular expressions from' $$log); " + "compiled=$$(grep -c 'Compiling regex:' $$log); " + "cached_primary=$$(grep 'Cached regular expressions from' $$log | grep -F -c '/regex_remap.conf'); " + "cached_secondary=$$(grep 'Cached regular expressions from' $$log | grep -F -c '/regex_remap2.conf'); " + "reused_primary=$$(grep 'Reusing cached regular expressions from' $$log | grep -F -c '/regex_remap.conf'); " + "reused_secondary=$$(grep 'Reusing cached regular expressions from' $$log | grep -F -c '/regex_remap2.conf'); " + "echo cached=$$cached reused=$$reused compiled=$$compiled " + "cached_primary=$$cached_primary cached_secondary=$$cached_secondary " + "reused_primary=$$reused_primary reused_secondary=$$reused_secondary; " + "test $$cached -eq 3 -a $$reused -eq 3 -a $$compiled -eq 6 -a " + "$$cached_primary -eq 2 -a $$cached_secondary -eq 1 -a " + "$$reused_primary -eq 2 -a $$reused_secondary -eq 1") + tr.Processes.Default.ReturnCode = 0 + tr.StillRunningAfter = self._ts + return tr + + +TestRegexRemapRuleCache(ts, ''.join(regex_remap_lines), curl_and_args)