bryancall opened a new issue, #13638: URL: https://github.com/apache/trafficserver/issues/13638
## 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()`](https://github.com/apache/trafficserver/blob/master/src/tsutil/Regex.cc#L505-L515) 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`](https://github.com/apache/trafficserver/blob/master/include/tsutil/Regex.h#L82) slots, so a pattern with more than about nine capture groups lands on that path. 3. [`Pattern::capture()`](https://github.com/apache/trafficserver/blob/master/plugins/cachekey/pattern.cc#L222) loops on the returned `matchCount` rather than on `matches.size()`: ```cpp 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()`](https://github.com/apache/trafficserver/blob/master/plugins/cachekey/pattern.cc#L153-L165) sees a size that is not 1 and takes the else branch: ```cpp 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](https://github.com/apache/trafficserver/pull/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. -- 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]
