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/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; 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); 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/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/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); 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) { 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)