prefetch: don't drop replacements for non-participating optional capture groups

doc/admin-guide/plugins/prefetch.en.rst

@@ -293,6 +293,13 @@ Plugin parameters
     * if ``true`` the fetch policy would use the **next** URL's cache key that to find out if the **next object** should be prefetched or not
 * ``--log-name`` - specifies a custom log name (if not specified a log is not created)
 
+An invalid parameter value is a configuration error. ``--fetch-count`` and ``--fetch-max`` must be
+decimal numbers that fit in an unsigned integer, ``--fetch-overflow`` must be ``32`` or ``64``, and
+``--fetch-path-pattern`` must compile and may only reference capture groups that the pattern
+defines. A remap rule with an invalid value fails to load, instead of loading with prefetch
+silently disabled. ``traffic_ctl config reload`` rejects such a configuration and keeps the running
+one.
+
 Metrics
 =======
 

plugins/prefetch/configs.cc

@@ -21,6 +21,8 @@
  * @brief Plugin configuration.
  */
 
+#include <charconv>  /* std::from_chars() */
+#include <cstring>   /* strlen() */
 #include <fstream>   /* std::ifstream */
 #include <getopt.h>  /* getopt_long() */
 #include <sstream>   /* std::istringstream */
@@ -71,14 +73,44 @@ iequals(const StringView lhs, const StringView rhs)
                     [](const char a, const char b) { return tolower(a) == tolower(b); });
 }
 
-void
+bool
 PrefetchConfig::setFetchOverflow(const char *optarg)
 {
-  if (StringView("64") == optarg) {
+  if (nullptr == optarg) {
+    return false;
+  }
+  if (StringView("32") == optarg) {
+    _fetchOverflow = EvalPolicy::Overflow32;
+  } else if (StringView("64") == optarg) {
     _fetchOverflow = EvalPolicy::Overflow64;
   } else if (iequals("bignum", optarg)) {
     _fetchOverflow = EvalPolicy::Bignum;
+  } else {
+    return false;
   }
+  return true;
+}
+
+/**
+ * @brief Parses @a optarg as an unsigned integer option value.
+ * @param optarg the option value to parse.
+ * @param value set to the parsed value on success, untouched on failure.
+ * @return true if @a optarg is a non-empty string of decimal digits that fits in @a value.
+ */
+static bool
+parseUnsignedInt(const char *optarg, unsigned &value)
+{
+  if (nullptr == optarg) {
+    return false;
+  }
+
+  const char *const end = optarg + strlen(optarg);
+  auto const [parsed, ec]{std::from_chars(optarg, end, value)};
+
+  /* from_chars() reports a leading sign or a non-digit as invalid_argument and a value too large
+   * for @a value as result_out_of_range. Requiring it to consume the whole string rejects trailing
+   * characters such as "10abc". */
+  return std::errc{} == ec && parsed == end;
 }
 
 /**
@@ -147,7 +179,12 @@ PrefetchConfig::init(int argc, char *argv[])
       break;
 
     case 'c': /* --fetch-count */
-      setFetchCount(optarg);
+      if (unsigned count = 0; parseUnsignedInt(optarg, count)) {
+        setFetchCount(count);
+      } else {
+        PrefetchError("invalid --fetch-count '%s': expected a non-negative integer", optarg ? optarg : "");
+        status = false;
+      }
       break;
 
     case 'e': /* --fetch-path-pattern */ {
@@ -156,7 +193,10 @@ PrefetchConfig::init(int argc, char *argv[])
         if (pattern->init(optarg)) {
           _nextPaths.add(std::move(pattern));
         } else {
-          PrefetchError("failed to initialize next object pattern");
+          /* An unusable fetch-path-pattern is a configuration error; fail instance creation so ATS
+           * refuses to load the remap rule rather than silently running with prefetch disabled. */
+          PrefetchError("failed to initialize fetch-path-pattern '%s'", optarg ? optarg : "");
+          status = false;
         }
       }
     } break;
@@ -166,11 +206,19 @@ PrefetchConfig::init(int argc, char *argv[])
     } break;
 
     case 'x': /* --fetch-max */
-      setFetchMax(optarg);
+      if (unsigned max = 0; parseUnsignedInt(optarg, max)) {
+        setFetchMax(max);
+      } else {
+        PrefetchError("invalid --fetch-max '%s': expected a non-negative integer", optarg ? optarg : "");
+        status = false;
+      }
       break;
 
     case 'o': /* --fetch-overflow */
-      setFetchOverflow(optarg);
+      if (!setFetchOverflow(optarg)) {
+        PrefetchError("invalid --fetch-overflow '%s': expected 32, 64, or bignum", optarg ? optarg : "");
+        status = false;
+      }
       break;
 
     case 'r': /* --replace-host */

plugins/prefetch/configs.h

@@ -23,6 +23,7 @@
 
 #pragma once
 
+#include <atomic>
 #include <string>
 
 #include "common.h"
@@ -119,9 +120,9 @@ class PrefetchConfig
   }
 
   void
-  setFetchCount(const char *optarg)
+  setFetchCount(unsigned count)
   {
-    _fetchCount = getValue(optarg);
+    _fetchCount = count;
   }
 
   unsigned
@@ -131,9 +132,9 @@ class PrefetchConfig
   }
 
   void
-  setFetchMax(const char *optarg)
+  setFetchMax(unsigned max)
   {
-    _fetchMax = getValue(optarg);
+    _fetchMax = max;
   }
 
   unsigned
@@ -142,7 +143,7 @@ class PrefetchConfig
     return _fetchMax;
   }
 
-  void setFetchOverflow(const char *optarg);
+  bool setFetchOverflow(const char *optarg);
 
   EvalPolicy
   getFetchOverflow() const
@@ -180,6 +181,20 @@ class PrefetchConfig
     return _nextPaths;
   }
 
+  /**
+   * @brief Whether an expanded path that collapsed to empty should be reported.
+   *
+   * Whether the replacement collapses depends on the request, so the condition recurs per
+   * transaction for as long as the pattern stays misconfigured. Report it once per remap instance
+   * so a bad pattern is visible without flooding the error log. A config reload builds a new
+   * instance and reports again.
+   */
+  bool
+  shouldReportEmptyPath()
+  {
+    return !_reportedEmptyPath.exchange(true, std::memory_order_relaxed);
+  }
+
   void
   setLogName(const char *optarg)
   {
@@ -226,4 +241,6 @@ class PrefetchConfig
   bool         _exactMatch    = false;
   bool         _cmcd_nor      = false;
   MultiPattern _nextPaths;
+
+  std::atomic<bool> _reportedEmptyPath{false}; /* see shouldReportEmptyPath() */
 };

plugins/prefetch/pattern.cc

@@ -130,45 +130,6 @@ Pattern::empty() const
   return _pattern.empty() || _regex.empty();
 }
 
-/**
- * @brief Capture or capture-and-replace depending on whether a replacement string is specified.
- * @see replace()
- * @see capture()
- * @param subject PCRE2 subject string
- * @param result vector of strings where the result of captures or the replacements will be returned.
- * @return true if there was a match and capture or replacement succeeded, false if failure.
- */
-bool
-Pattern::process(const String &subject, StringVector &result)
-{
-  if (!_replacement.empty()) {
-    /* Replacement pattern was provided in the configuration - capture and replace. */
-    String element;
-    if (replace(subject, element)) {
-      result.push_back(element);
-    } else {
-      return false;
-    }
-  } else {
-    /* Replacement was not provided so return all capturing groups except the group zero. */
-    StringVector captures;
-    if (capture(subject, captures)) {
-      if (captures.size() == 1) {
-        result.push_back(captures[0]);
-      } else {
-        StringVector::iterator it = captures.begin() + 1;
-        for (; it != captures.end(); it++) {
-          result.push_back(*it);
-        }
-      }
-    } else {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 /**
  * @brief PCRE2 matches a subject string against the regex pattern.
  * @param subject PCRE2 subject
@@ -195,39 +156,6 @@ Pattern::match(const String &subject)
   return true;
 }
 
-/**
- * @brief Return all PCRE2 capture groups that matched in the subject string
- * @param subject PCRE2 subject string
- * @param result reference to vector of strings containing all capture groups
- */
-bool
-Pattern::capture(const String &subject, StringVector &result)
-{
-  PrefetchDebug("matching '%s' to '%s'", _pattern.c_str(), subject.c_str());
-
-  if (_regex.empty()) {
-    return false;
-  }
-
-  RegexMatches matches;
-  int          matchCount = _regex.exec(subject, matches, RE_NOTEMPTY);
-
-  if (matchCount <= 0) {
-    if (matchCount != RE_ERROR_NOMATCH) {
-      PrefetchError("matching error %d", matchCount);
-    }
-    return false;
-  }
-
-  for (int i = 0; i < matchCount; i++) {
-    std::string_view match = matches[i];
-    result.emplace_back(match.data(), match.length());
-    PrefetchDebug("capturing '%s' %d", result.back().c_str(), i);
-  }
-
-  return true;
-}
-
 /**
  * @brief Replaces all replacements found in the replacement string with what matched in the PCRE2 capturing groups.
  * @param subject PCRE2 subject string
@@ -253,25 +181,22 @@ Pattern::replace(const String &subject, String &result)
     return false;
   }
 
-  /* Verify the replacement has the right number of matching groups */
-  for (int i = 0; i < _tokenCount; i++) {
-    if (_tokens[i] >= matchCount) {
-      PrefetchError("invalid reference in replacement string: $%d", _tokens[i]);
-      return false;
-    }
-  }
-
   int previous = 0;
   for (int i = 0; i < _tokenCount; i++) {
-    int              replIndex = _tokens[i];
-    std::string_view dst       = matches[replIndex];
+    int replIndex = _tokens[i];
 
-    String src(_replacement, _tokenOffset[i], 2);
+    /* $replIndex was validated at config-load time against the number of groups the pattern defines, but
+     * the group may still not have participated in *this* match (e.g. a trailing optional group such as
+     * "(\?.*)?" when the subject has no query string).  pcre2_match() returns one past the highest
+     * participating group, so substitute an empty string for a group at or beyond that -- the documented
+     * PCRE2 semantics for an unmatched group -- rather than failing the whole replacement.  Use ""
+     * rather than a default-constructed view so data() is never null, which "%.*s" requires. */
+    std::string_view dst = (replIndex < matchCount) ? matches[replIndex] : std::string_view{""};
 
-    PrefetchDebug("replacing '%s' with '%.*s'", src.c_str(), static_cast<int>(dst.length()), dst.data());
+    PrefetchDebug("replacing '$%d' with '%.*s'", replIndex, static_cast<int>(dst.length()), dst.data());
 
     result.append(_replacement, previous, _tokenOffset[i] - previous);
-    result.append(dst.data(), dst.length());
+    result.append(dst);
 
     previous = _tokenOffset[i] + 2; /* 2 is the size of $0 or $1 or $2, ... or $9 */
   }
@@ -331,6 +256,31 @@ Pattern::compile()
     }
   }
 
+  /* Validate replacement references against the number of capture groups the pattern actually defines
+   * (not how many happen to participate in any given match) at config-load time.  This catches a
+   * genuinely out-of-range reference such as $5 against a 3-group pattern, and a pattern that defines
+   * more groups than can be captured -- RegexMatches holds the whole match plus TOKENCOUNT-1 groups. */
+  if (success) {
+    int32_t captureCount = _regex.get_capture_count();
+    if (captureCount < 0) {
+      PrefetchError("failed to get capture count for regex '%s'", _pattern.c_str());
+      success = false;
+    } else if (captureCount > TOKENCOUNT - 1) {
+      PrefetchError("regex '%s' defines %d capture groups; the prefetch plugin supports at most %d (references $0..$%d)",
+                    _pattern.c_str(), captureCount, TOKENCOUNT - 1, TOKENCOUNT - 1);
+      success = false;
+    } else {
+      for (int i = 0; i < _tokenCount; i++) {
+        if (_tokens[i] > captureCount) {
+          PrefetchError("invalid reference $%d in replacement '%s': pattern defines only %d group(s)", _tokens[i],
+                        _replacement.c_str(), captureCount);
+          success = false;
+          break;
+        }
+      }
+    }
+  }
+
   return success;
 }
 

plugins/prefetch/pattern.h

@@ -40,9 +40,7 @@ class Pattern
   bool init(const String &config);
   bool empty() const;
   bool match(const String &subject);
-  bool capture(const String &subject, StringVector &result);
   bool replace(const String &subject, String &result);
-  bool process(const String &subject, StringVector &result);
 
 private:
   bool compile();

plugins/prefetch/plugin.cc

@@ -663,6 +663,19 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata)
               String expandedPath;
 
               if (config.getNextPath().replace(workingPath, expandedPath)) {
+                if (expandedPath.empty()) {
+                  /* A replacement that collapses to empty (e.g. every referenced group was optional and
+                   * absent) would otherwise be scheduled with a zero-length path, which BgFetch skips --
+                   * leaving the original request path in place and prefetching the pristine URL itself.
+                   * Stop rather than issue that self-prefetch. Report once per instance; whether the
+                   * replacement collapses depends on the request, so this recurs per transaction. */
+                  if (config.shouldReportEmptyPath()) {
+                    PrefetchError("prefetch pattern produced an empty path; check the fetch-path-pattern replacement");
+                  } else {
+                    PrefetchDebug("prefetch pattern produced an empty path");
+                  }
+                  break;
+                }
                 PrefetchDebug("replaced: %s", expandedPath.c_str());
                 expand(expandedPath, config.getFetchOverflow());
                 PrefetchDebug("expanded: %s cachekey: %s", expandedPath.c_str(), data->_cachekey.c_str());