This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new adbd85d870 prefetch: don't drop replacements for non-participating
optional capture groups (#13352)
adbd85d870 is described below
commit adbd85d8707a674538a7ee2fe8816a8ab07c159d
Author: Mo Chen <[email protected]>
AuthorDate: Mon Aug 10 11:34:17 2026 -0500
prefetch: don't drop replacements for non-participating optional capture
groups (#13352)
Pattern::replace() rejected any $N whose index was >= the match's
return value. That value is one past the highest capture group that
*participated*, not the number of groups the pattern defines, so a
trailing optional group such as "(\?.*)?" that did not participate made
a valid $N look out of range. Every such request logged "invalid
reference in replacement string" and silently dropped the prefetch.
Validate $N once at config-load time against the pattern's actual
capture-group count, and substitute an empty string at match time for
a group that did not participate, per PCRE2 semantics.
Also treat an unusable --fetch-path-pattern and invalid --fetch-count
/ --fetch-max / --fetch-overflow values as configuration errors, so
the remap rule is refused at load rather than running with prefetch
silently disabled. --fetch-count and --fetch-max now parse with
std::from_chars(), which rejects a value above UINT_MAX instead of
truncating it. Skip an empty expanded path rather than self-
prefetching the original, and report that once per remap instance
since the condition is request-dependent.
Removes the now-dead Pattern::process() and Pattern::capture().
---
doc/admin-guide/plugins/prefetch.en.rst | 7 ++
plugins/prefetch/configs.cc | 60 +++++++++--
plugins/prefetch/configs.h | 27 ++++-
plugins/prefetch/pattern.cc | 120 ++++++---------------
plugins/prefetch/pattern.h | 2 -
plugins/prefetch/plugin.cc | 13 +++
.../prefetch/prefetch_bad_count_refused.test.py | 53 +++++++++
.../prefetch/prefetch_bad_pattern_refused.test.py | 54 ++++++++++
.../prefetch/prefetch_empty_replacement.test.py | 90 ++++++++++++++++
.../prefetch/prefetch_optional_group.gold | 4 +
.../prefetch/prefetch_optional_group.test.py | 95 ++++++++++++++++
11 files changed, 427 insertions(+), 98 deletions(-)
diff --git a/doc/admin-guide/plugins/prefetch.en.rst
b/doc/admin-guide/plugins/prefetch.en.rst
index 4f6bf7a759..62efb348c9 100644
--- a/doc/admin-guide/plugins/prefetch.en.rst
+++ b/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
=======
diff --git a/plugins/prefetch/configs.cc b/plugins/prefetch/configs.cc
index 8d74e7585e..9e86a9a2bc 100644
--- a/plugins/prefetch/configs.cc
+++ b/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 */
diff --git a/plugins/prefetch/configs.h b/plugins/prefetch/configs.h
index e36e12cdb7..3ad3980f38 100644
--- a/plugins/prefetch/configs.h
+++ b/plugins/prefetch/configs.h
@@ -23,6 +23,7 @@
#pragma once
+#include <atomic>
#include <string>
#include "common.h"
@@ -119,9 +120,9 @@ public:
}
void
- setFetchCount(const char *optarg)
+ setFetchCount(unsigned count)
{
- _fetchCount = getValue(optarg);
+ _fetchCount = count;
}
unsigned
@@ -131,9 +132,9 @@ public:
}
void
- setFetchMax(const char *optarg)
+ setFetchMax(unsigned max)
{
- _fetchMax = getValue(optarg);
+ _fetchMax = max;
}
unsigned
@@ -142,7 +143,7 @@ public:
return _fetchMax;
}
- void setFetchOverflow(const char *optarg);
+ bool setFetchOverflow(const char *optarg);
EvalPolicy
getFetchOverflow() const
@@ -180,6 +181,20 @@ public:
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 @@ private:
bool _exactMatch = false;
bool _cmcd_nor = false;
MultiPattern _nextPaths;
+
+ std::atomic<bool> _reportedEmptyPath{false}; /* see shouldReportEmptyPath()
*/
};
diff --git a/plugins/prefetch/pattern.cc b/plugins/prefetch/pattern.cc
index 22926c42f4..f73a7ff224 100644
--- a/plugins/prefetch/pattern.cc
+++ b/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;
}
diff --git a/plugins/prefetch/pattern.h b/plugins/prefetch/pattern.h
index 1105c17305..43f8ea5b15 100644
--- a/plugins/prefetch/pattern.h
+++ b/plugins/prefetch/pattern.h
@@ -40,9 +40,7 @@ public:
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();
diff --git a/plugins/prefetch/plugin.cc b/plugins/prefetch/plugin.cc
index 37c8d60a17..22fe984f3d 100644
--- a/plugins/prefetch/plugin.cc
+++ b/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());
diff --git
a/tests/gold_tests/pluginTest/prefetch/prefetch_bad_count_refused.test.py
b/tests/gold_tests/pluginTest/prefetch/prefetch_bad_count_refused.test.py
new file mode 100644
index 0000000000..421e07e1c5
--- /dev/null
+++ b/tests/gold_tests/pluginTest/prefetch/prefetch_bad_count_refused.test.py
@@ -0,0 +1,53 @@
+'''
+'''
+# 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 a --fetch-count that does not fit in an unsigned
as a configuration
+error: ATS refuses to load the remap rule (and fails to start) instead of
truncating the value.
+
+The value below is a string of decimal digits, so it passes a digits-only
check, but it exceeds the
+range of the unsigned the plugin stores it in.
+'''
+
+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" + " @pparam=--fetch-count=5000000000")
+
+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(
+ "invalid --fetch-count '5000000000'", "an out-of-range fetch-count must be
rejected at config load")
+
+tr = Test.AddTestRun("prefetch rejects an out-of-range fetch-count 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, "invalid
--fetch-count '5000000000'")
+watcher.StartBefore(ts)
+
+tr.Processes.Default.Command = "echo done"
+tr.TimeOut = 30
+tr.Processes.Default.StartBefore(watcher)
diff --git
a/tests/gold_tests/pluginTest/prefetch/prefetch_bad_pattern_refused.test.py
b/tests/gold_tests/pluginTest/prefetch/prefetch_bad_pattern_refused.test.py
new file mode 100644
index 0000000000..01ddbfd1a2
--- /dev/null
+++ b/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)
diff --git
a/tests/gold_tests/pluginTest/prefetch/prefetch_empty_replacement.test.py
b/tests/gold_tests/pluginTest/prefetch/prefetch_empty_replacement.test.py
new file mode 100644
index 0000000000..4556e52b59
--- /dev/null
+++ b/tests/gold_tests/pluginTest/prefetch/prefetch_empty_replacement.test.py
@@ -0,0 +1,90 @@
+'''
+'''
+# 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 prefetch.so does not self-prefetch when a valid pattern's replacement
collapses to an empty path.
+
+The replacement here is just "$3", where group 3 ("(\\?.*)?") is optional.
For a query-less request
+that group is absent, so the replacement expands to the empty string. An
empty expanded path would
+otherwise be scheduled with zero length, which BgFetch skips -- leaving the
pristine request path and
+prefetching the URL itself. The plugin must instead log and stop; the client
request is still served.
+'''
+
+server = Test.MakeOriginServer("server")
+for i in list(range(1, 1 + 2)):
+ request_header = {
+ "headers":
+ f"GET /texts/demo-{i} HTTP/1.1\r\n"
+ "Host: does.not.matter\r\n" # But cannot be omitted.
+ "\r\n",
+ "timestamp": "1469733493.993",
+ "body": ""
+ }
+ response_header = {
+ "headers": "HTTP/1.1 200 OK\r\n"
+ "Connection: close\r\n"
+ "Cache-control: max-age=85000\r\n"
+ "\r\n",
+ "timestamp": "1469733493.993",
+ "body": f"This is the body for demo-{i}.\n"
+ }
+ server.addResponse("sessionlog.json", request_header, response_header)
+
+dns = Test.MakeDNServer("dns")
+
+ts = Test.MakeATSProcess("ts")
+ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http|dns|prefetch',
+ 'proxy.config.dns.nameservers': f"127.0.0.1:{dns.Variables.Port}",
+ 'proxy.config.dns.resolv_conf': "NULL",
+ })
+# A valid pattern (3 defined groups, $3 in range) whose replacement is only
the optional $3 group.
+ts.Disk.remap_config.AddLine(
+ f"map http://domain.in http://127.0.0.1:{server.Variables.Port}"; + "
@plugin=cachekey.so @pparam=--remove-all-params=true"
+ " @plugin=prefetch.so" + " @pparam=--front=true" + "
@pparam=--fetch-policy=simple" +
+ r" @pparam=--fetch-path-pattern=/(.*-)(\d+)(\?.*)?$/$3/" + "
@pparam=--fetch-count=3")
+ts.ReturnCode = Any(0, -2)
+
+# The empty replacement must be logged and skipped. This ContainsExpression
both asserts the message
+# and replaces the default "diags.log must not contain ERROR:" check (it is
logged via TSError).
+ts.Disk.diags_log.Content = Testers.ContainsExpression(
+ "produced an empty path", "an empty replacement must be logged and
skipped, not self-prefetched")
+
+tr = Test.AddTestRun()
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(dns)
+tr.Processes.Default.StartBefore(ts)
+tr.Processes.Default.Command = 'echo start TS, HTTP server and DNS.'
+tr.Processes.Default.ReturnCode = 0
+
+# The client request is still served normally even though the prefetch is
skipped.
+tr = Test.AddTestRun()
+tr.MakeCurlCommand(f'--verbose --proxy 127.0.0.1:{ts.Variables.port}
http://domain.in/texts/demo-1')
+tr.Processes.Default.ReturnCode = 0
+
+Test.AddAwaitFileContainsTestRun('Await the empty-path skip to be logged.',
ts.Disk.diags_log.Name, 'produced an empty path')
+
+# The self-prefetch that the old code issued would re-fetch the original path.
With the fix the loop
+# stops before scheduling, so "failed to process the pattern" (the old
second-iteration symptom on the
+# emptied working path) must never appear.
+tr = Test.AddTestRun()
+tr.Processes.Default.Command = (f"grep -c 'failed to process the pattern'
{ts.Disk.traffic_out.Name} || true")
+tr.Streams.stdout = Testers.ContainsExpression("0", "no per-request
pattern-processing failure")
+tr.Processes.Default.ReturnCode = 0
diff --git a/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.gold
b/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.gold
new file mode 100644
index 0000000000..5cd6cb5de9
--- /dev/null
+++ b/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.gold
@@ -0,0 +1,4 @@
+GET http://domain.in/texts/demo-1 HTTP/1.1
+GET http://domain.in/texts/demo-2 HTTP/1.1
+GET http://domain.in/texts/demo-3 HTTP/1.1
+GET http://domain.in/texts/demo-4 HTTP/1.1
diff --git
a/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.test.py
b/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.test.py
new file mode 100644
index 0000000000..813a6b640a
--- /dev/null
+++ b/tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.test.py
@@ -0,0 +1,95 @@
+'''
+'''
+# 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 prefetch.so with an optional trailing capture group that does not
participate in the match.
+
+The fetch-path-pattern below ends in an optional "(\\?.*)?" group that only
participates when the
+subject carries a query string. For a query-less request that group is
absent, so pcre_exec() returns
+3 -- one past the highest *participating* group -- even though the pattern
defines a 3rd group that the
+replacement references as $3. The old code compared $3 against that return
value and wrongly rejected
+it; a non-participating group must instead substitute an empty string rather
than fail the whole
+replacement (which previously logged "invalid reference in replacement string:
$3" and silently
+dropped every prefetch). Mirrors the production hls/mvod remap pattern
+"/(.*-)(\\.m3u8)(\\?.*)?$/$1-0.mp4$3/".
+'''
+
+server = Test.MakeOriginServer("server")
+for i in list(range(1, 1 + 4)):
+ request_header = {
+ "headers":
+ f"GET /texts/demo-{i} HTTP/1.1\r\n"
+ "Host: does.not.matter\r\n" # But cannot be omitted.
+ "\r\n",
+ "timestamp": "1469733493.993",
+ "body": ""
+ }
+ response_header = {
+ "headers": "HTTP/1.1 200 OK\r\n"
+ "Connection: close\r\n"
+ "Cache-control: max-age=85000\r\n"
+ "\r\n",
+ "timestamp": "1469733493.993",
+ "body": f"This is the body for demo-{i}.\n"
+ }
+ server.addResponse("sessionlog.json", request_header, response_header)
+
+dns = Test.MakeDNServer("dns")
+
+ts = Test.MakeATSProcess("ts")
+ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http|dns|prefetch',
+ 'proxy.config.dns.nameservers': f"127.0.0.1:{dns.Variables.Port}",
+ 'proxy.config.dns.resolv_conf': "NULL",
+ })
+ts.Disk.remap_config.AddLine(
+ f"map http://domain.in http://127.0.0.1:{server.Variables.Port}"; + "
@plugin=cachekey.so @pparam=--remove-all-params=true"
+ " @plugin=prefetch.so" + " @pparam=--front=true" + "
@pparam=--fetch-policy=simple" +
+ r" @pparam=--fetch-path-pattern=/(.*-)(\d+)(\?.*)?$/$1{$2+1}$3/" + "
@pparam=--fetch-count=3")
+ts.ReturnCode = Any(0, -2)
+
+# Belt-and-suspenders next to the gold comparison (which is the primary
guard): a regression that
+# re-introduces a per-request rejection of the non-participating $3 logs an
"invalid reference ..."
+# error. This pattern is valid (3 defined groups, $3 in range) so the
compile-time validator never
+# fires either, hence no "invalid reference" text should ever reach the log.
+ts.Disk.traffic_out.Content = Testers.ExcludesExpression(
+ "invalid reference", "optional non-participating group must not fail the
replacement")
+
+tr = Test.AddTestRun()
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(dns)
+tr.Processes.Default.StartBefore(ts)
+tr.Processes.Default.Command = 'echo start TS, HTTP server and DNS.'
+tr.Processes.Default.ReturnCode = 0
+
+tr = Test.AddTestRun()
+tr.MakeCurlCommand(f'--verbose --proxy 127.0.0.1:{ts.Variables.port}
http://domain.in/texts/demo-1')
+tr.Processes.Default.ReturnCode = 0
+
+# The original request and the three prefetches are logged independently and
may finish out of
+# order, so wait for every expected URL to be logged before comparing, and
sort both sides so the
+# comparison does not depend on completion order.
+for tag in ['demo-1', 'demo-2', 'demo-3', 'demo-4']:
+ Test.AddAwaitFileContainsTestRun(f'Await {tag} to be logged.',
ts.Disk.traffic_out.Name, tag)
+
+tr = Test.AddTestRun()
+tr.Processes.Default.Command = (f"grep 'GET http://domain.in'
{ts.Disk.traffic_out.Name} | sort")
+tr.Streams.stdout = "prefetch_optional_group.gold"
+tr.Processes.Default.ReturnCode = 0
