Copilot commented on code in PR #13352:
URL: https://github.com/apache/trafficserver/pull/13352#discussion_r3667123029
##########
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:
When a group doesn’t participate, `dst` becomes a default-constructed
`std::string_view{}` whose `data()` may be `nullptr`. Passing a null pointer to
`'%.*s'` and calling `result.append(dst.data(), dst.length())` is undefined
behavior even when the length is 0. Use a non-null empty buffer (e.g.,
`std::string_view{\"\"}`) and/or skip appending/logging via `%.*s` when
`dst.empty()` (or normalize the pointer to `\"\"`).
##########
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:
When a group doesn’t participate, `dst` becomes a default-constructed
`std::string_view{}` whose `data()` may be `nullptr`. Passing a null pointer to
`'%.*s'` and calling `result.append(dst.data(), dst.length())` is undefined
behavior even when the length is 0. Use a non-null empty buffer (e.g.,
`std::string_view{\"\"}`) and/or skip appending/logging via `%.*s` when
`dst.empty()` (or normalize the pointer to `\"\"`).
--
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]
