moonchen commented on code in PR #13352:
URL: https://github.com/apache/trafficserver/pull/13352#discussion_r3666806853
##########
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{};
Review Comment:
Confirmed against PCRE2 10.47: for `(a)?(b)` on subject `b`, the match count
is 3 and group 1 is `PCRE2_UNSET`, so `replIndex < matchCount` passes and the
subject pointer is advanced by an unset offset.
Two corrections. This is not introduced here — master reaches
`matches[replIndex]` under the identical `_tokens[i] >= matchCount`
precondition, so the exposure is the same before and after this PR. And it does
not crash: the length is `end - start` with both unset, so the view is
zero-length and callers see an empty string. The pointer is invalid, which is
worth fixing, but nothing observably breaks.
The real defect is in `RegexMatches::operator[]`, which every ovector caller
shares (`regex_remap`, `cachekey`, `regex_revalidate`). Fixing it there rather
than in this plugin: #13441.
--
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]
