Handle all cachekey regex capture groups

[bneradt]

Cachekey patterns with ten or more capture groups can crash ATS when
building a cache key because a successful match leaves the capture
vector empty. Replacement patterns also reject valid group references
when the default match buffer is too small.

This patch sizes match buffers for the complete pattern, uses the
available match count, and rejects empty capture results before
iteration. Replay coverage verifies complete cache keys across the
capture limit and preserves replacement and no-match behavior.

Fixes: #13638

Co-authored-by: GPT-6 Astra Light

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bryancall]

Requesting changes, on one narrow point. The core fix is right and I verified it against PCRE2 directly rather than reading it off the code: with a 10-pair match buffer, pcre2_match() returns 10 for a 9-group pattern but 0 for 10 and 12 groups, meaning "matched, ovector too small". The old loop then ran zero times, leaving captures empty, and Pattern::process() did captures.begin() + 1 on an empty vector. Sizing the buffer from get_capture_count() + 1 fixes the cause rather than the symptom, and the replay test verifies full cache keys with as: equal while bracketing the boundary at 9, 10 and 12 groups. Good test design.

The $N bound in replace() is still wrong, and you are already editing that line

matches.size() is the pcre2_match() return value, so it is one past the highest group that participated, not the number the pattern defines. For /(.*-)(\d+)(\?.*)?$/$1$3/ against a subject with no query string, the trailing group does not participate, $3 looks out of range, and the whole replacement is rejected with "invalid reference in replacement string". The element is then silently dropped from the cache key.

This predates your patch, so I would normally send it off as its own issue. I am asking for it here because the change at line 260 swaps matchCount for matches.size(), and those two are equal once the buffer is sized correctly, so that edit is a no-op sitting on top of the actual bug. Fixing the bound is less work than leaving it half-corrected.

There is a merged in-tree precedent to copy: #13352 fixed the identical bug in the prefetch plugin. It validates $N once at config-load time against the pattern's real capture count, then substitutes an empty string at match time for a group that did not participate, per PCRE2 semantics. plugins/prefetch/pattern.cc is worth reading side by side, and the comment above its substitution explains the ""-rather-than-default-view detail that keeps data() non-null.

For cachekey the insertion point is cleaner than it was for prefetch, since Pattern::compile() already does both the regex compile and the $N token parse in one function. At the end of it:

int32_t const captureCount = _re.get_capture_count();
if (captureCount < 0) {
  CacheKeyError("failed to get capture count for pattern '%s'", _pattern.c_str());
  return false;
}
for (int i = 0; i < _tokenCount; i++) {
  if (_tokens[i] > captureCount) {
    CacheKeyError("invalid reference $%d in replacement '%s': pattern defines only %d group(s)", _tokens[i],
                  _replacement.c_str(), captureCount);
    return false;
  }
}

Then drop the runtime validation loop at line 259 and clamp the lookup:

std::string_view capture = (replIndex < matches.size()) ? matches[replIndex] : std::string_view{""};

That also closes something the current diff opens on its own: get_capture_count() returns -1 when pcre2_pattern_info() fails, and -1 + 1 == 0 into the uint32_t size parameter gives a single-pair buffer, which captures group zero only and yields a silently wrong cache key. The _re.empty() check above makes it unlikely rather than impossible. The guard above handles it.

Why this is worth the extra lines

Both loop-bound changes in the PR are currently unfalsifiable. With the buffer sized correctly, matchCount == matches.size() on every successful match, so reverting either one leaves all six replay cases green. Fixing the bound is what makes them load-bearing, and the optional-group case is what tests them. Your replay file already exists, so it is one more remap rule and one more transaction.

Notes, no action needed

Three of the six cases are regression tests. The 9-group, whole-pattern and no-match cases pass on unpatched master, which is fine, they are boundary controls, and the 9-group one sits exactly at the old capacity so it is worth keeping.

The 12-group case is the first cachekey test to push RegexMatches past its 400-byte inline buffer onto ::malloc, so this adds a per-transaction heap allocation for patterns with 10 or more groups. Worth a line in the description, and worth running that case under ASan.