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

plugins/prefetch/configs.cc

@@ -71,14 +71,39 @@ 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 Whether @a optarg is a non-empty string of decimal digits (a valid unsigned integer option).
+ */
+static bool
+isUnsignedInt(const char *optarg)
+{
+  if (nullptr == optarg || '\0' == *optarg) {
+    return false;
+  }
+  for (const char *p = optarg; '\0' != *p; ++p) {
+    if (*p < '0' || *p > '9') {
+      return false;
+    }
   }
+  return true;
 }
 
 /**
@@ -147,7 +172,12 @@ PrefetchConfig::init(int argc, char *argv[])
       break;
 
     case 'c': /* --fetch-count */
-      setFetchCount(optarg);
+      if (isUnsignedInt(optarg)) {
+        setFetchCount(optarg);
+      } else {
+        PrefetchError("invalid --fetch-count '%s': expected a non-negative integer", optarg ? optarg : "");
+        status = false;
+      }
       break;
 
     case 'e': /* --fetch-path-pattern */ {
@@ -156,7 +186,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 +199,19 @@ PrefetchConfig::init(int argc, char *argv[])
     } break;
 
     case 'x': /* --fetch-max */
-      setFetchMax(optarg);
+      if (isUnsignedInt(optarg)) {
+        setFetchMax(optarg);
+      } 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

@@ -142,7 +142,7 @@ class PrefetchConfig
     return _fetchMax;
   }
 
-  void setFetchOverflow(const char *optarg);
+  bool setFetchOverflow(const char *optarg);
 
   EvalPolicy
   getFetchOverflow() const

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,22 +181,18 @@ 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. */
+    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());
@@ -331,6 +255,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

@@ -634,6 +634,14 @@ 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. */
+                  PrefetchError("prefetch pattern produced an empty path; check the fetch-path-pattern replacement");
+                  break;
+                }
                 PrefetchDebug("replaced: %s", expandedPath.c_str());
                 expand(expandedPath, config.getFetchOverflow());
                 PrefetchDebug("expanded: %s cachekey: %s", expandedPath.c_str(), data->_cachekey.c_str());

tests/gold_tests/pluginTest/prefetch/prefetch_bad_pattern_refused.test.py

@@ -0,0 +1,54 @@
+'''
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+Test.Summary = '''
+Test that prefetch.so treats an unusable fetch-path-pattern as a configuration error: ATS refuses to
+load the remap rule (and fails to start) instead of silently running with prefetch disabled.
+
+The pattern below defines 10 capture groups.  The plugin's ovector (OVECOUNT = TOKENCOUNT*3) can hold
+offsets for the whole match plus at most TOKENCOUNT-1 (9) groups, so the pattern is rejected at
+config-load time; that fails the remap instance, and remap.config fails to load.
+'''
+
+ts = Test.MakeATSProcess("ts")
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'prefetch',
+})
+ts.Disk.remap_config.AddLine(
+    "map http://domain.in http://127.0.0.1:8080" + " @plugin=prefetch.so" + " @pparam=--front=true" +
+    " @pparam=--fetch-policy=simple" + r" @pparam=--fetch-path-pattern=/(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)/$1/")
+
+ts.ReturnCode = 33  # Emergency exit: remap.config failed to load.
+ts.Ready = 0
+# ATS is expected to log the rejection; this ContainsExpression both asserts it and replaces the
+# default "diags.log must not contain ERROR:" check (the rejection is logged via TSError).
+ts.Disk.diags_log.Content = Testers.ContainsExpression(
+    "defines 10 capture groups", "over-limit fetch-path-pattern must be rejected at config load")
+
+tr = Test.AddTestRun("prefetch rejects an over-limit capture-group pattern at load")
+# Wait for the rejection message with a separate watcher: gating ts readiness on the log line directly
+# can race the process exiting before autest observes the line.
+watcher = Test.Processes.Process("watcher")
+watcher.Command = "sleep 30"
+watcher.Ready = When.FileContains(ts.Disk.diags_log.Name, "defines 10 capture groups")
+watcher.StartBefore(ts)
+
+tr.Processes.Default.Command = "echo done"
+tr.TimeOut = 30
+tr.Processes.Default.StartBefore(watcher)