moonchen commented on code in PR #13352:
URL: https://github.com/apache/trafficserver/pull/13352#discussion_r3737210530
##########
plugins/prefetch/pattern.cc:
##########
@@ -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];
Review Comment:
Fixed. The view is now built from `""` so `data()` is never null, and the
append uses the `std::string_view` overload rather than the pointer/length
pair, so neither the `"%.*s"` nor the append passes a null pointer.
##########
plugins/prefetch/pattern.cc:
##########
@@ -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());
Review Comment:
Fixed. The view is now built from `""` so `data()` is never null, and the
append uses the `std::string_view` overload rather than the pointer/length
pair, so neither the `"%.*s"` nor the append passes a null pointer.
##########
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");
Review Comment:
Good catch, and worse than per transaction: whether the replacement
collapses depends on the request, so for a misconfigured pattern it recurs
indefinitely rather than being a one-time complaint at load.
`PrefetchConfig::shouldReportEmptyPath()` now reports it at error level once
per remap instance and drops to the debug tag afterwards. A config reload
builds a new instance, so a genuine misconfiguration stays visible after each
reload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
