cachekey: Pattern::capture() can return true with an empty vector, and Pattern::process() reads past the end

[bryancall]

Summary

Pattern::capture() in the cachekey plugin can return true with an empty result vector. Pattern::process() then does captures.begin() + 1 on that empty vector and iterates, which reads past the end. This is reachable from a cachekey configuration that uses a regex with more than about nine capture groups.

The chain

1. Regex::exec() returns pcre2_match's rc verbatim. It sets matches._size = rc, and when rc == 0 it repairs _size from pcre2_get_ovector_count(), but the return value stays 0. A 0 from pcre2_match means the ovector was too small to hold every capture group.

2. A default-constructed RegexMatches has DEFAULT_MATCHES = 10 slots, so a pattern with more than about nine capture groups lands on that path.

3. Pattern::capture() loops on the returned matchCount rather than on matches.size():

int matchCount = _re.exec(subject, matches, RE_NOTEMPTY);
if (matchCount < 0) {
  ...
  return false;
}

for (int i = 0; i < matchCount; i++) {   // 0 iterations when matchCount == 0
  ...
  result.push_back(std::move(dst));
}

return true;                             // reports success with an empty vector

Only a negative return is treated as failure, so matchCount == 0 falls through and the function returns true having pushed nothing.

4. Pattern::process() sees a size that is not 1 and takes the else branch:

StringVector captures;
if (capture(subject, captures)) {
  if (captures.size() == 1) {
    result.push_back(std::move(captures[0]));
  } else {
    StringVector::iterator it = captures.begin() + 1;   // empty vector
    for (; it != captures.end(); it++) {
      result.push_back(*it);
    }
  }
}

On an empty vector begin() and end() are equal, so begin() + 1 is already past the end, and the it != captures.end() condition is false only if the iterator happens to land back on end(), which it never does going forward. The loop has no bound that stops it after one element.

Proposed fix

Two independent changes:

• capture() should loop on matches.size() rather than on the return value of exec(). That also stops it from silently dropping capture groups on a match where the ovector had to be resized.
• process() should check captures.empty() rather than trusting begin() + 1.

Worth deciding separately whether capture() should treat a 0 return as a failure, or whether RegexMatches should be constructed with a slot count derived from the pattern's capture count so the resize path is not hit in the first place.

Context

Noticed while reviewing PR #13591, which turns copies into moves in this plugin. That PR originally changed result.push_back(*it) on the loop body above to result.push_back(std::move(*it)), which would have turned the out-of-bounds read into an out-of-bounds write. The move was reverted on that line so the PR stays purely mechanical, and the underlying bug is filed here.