You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "moonchen (via GitHub)" <gi...@apache.org> on 2026/08/07 19:34:11 UTC
[PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
moonchen opened a new pull request, #13517:
URL: https://github.com/apache/trafficserver/pull/13517
## Problem
`RegexMatches` allocates its ovector with a fixed 10 pairs regardless of the pattern.
`pcre2_match()` fills it in only as far as the highest capture group that participated,
and leaves the rest untouched — holding whatever was already in `_buffer`, which is an
uninitialized member.
`operator[]` checked the index against the *allocated* pair count. So an index past the
pattern's groups passed the check and built a `string_view` out of that leftover memory:
a bad pointer with a meaningless length.
The `PCRE2_UNSET` check added in #13441 doesn't cover this. It catches a group the match
reached but that didn't participate. Entries the match never reached hold stale bytes,
not `PCRE2_UNSET`.
The same is true after a failed match, where PCRE2 leaves the ovector undefined. On
master, `matches[0]` after a failed `exec()` hands back a view over garbage.
Nothing in tree can reach this today: `regex_remap` rejects a `$n` above the pattern's
capture count at config load, prefetch and cachekey bound the index by the match count,
and `SSLSNIConfig` iterates to `matches.size()`. This fixes the contract, not a live bug.
## Fix
Bound the index by the match size — what `pcre2_match()` actually populated — rather than
the allocated pair count. Keep the `PCRE2_UNSET` check for a group inside that range that
didn't participate, which is the case #13441 was about.
Also return `""` rather than a default-constructed `std::string_view` for both empty
cases, so the result never has a null `data()`. Callers pass it straight into functions
that don't accept a null pointer even at zero length:
- `plugins/regex_remap/regex_remap.cc:540` — `memcpy()`
- `plugins/cachekey/pattern.cc:271` — `std::string` ctor
- `plugins/prefetch/pattern.cc:267` — `std::string::append()`
- `plugins/experimental/access_control/pattern.cc:287` — `std::string` ctor
`get_ovector_pointer()` is still available to tell a group that didn't participate from
one that matched an empty string, which is what the PCRE2 docs suggest for that.
## Why there's no check on the end offset
Raised in review on #13441, so worth answering here:
- PCRE2 sets both offsets of an unused group to `PCRE2_UNSET` together, so testing the
start is enough.
- `end < start` would need `\K` inside a lookaround, which PCRE2 has rejected at compile
time since 10.38 unless `PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK` is set. ATS never sets it,
and with it forced on PCRE2 clamps rather than inverting.
## Tests
Three sections in `test_Regex.cc`, all of which fail without the change:
- a non-participating group before a participating one — updated, the view is no longer null
- an index past what the match populated — new
- indexing after a failed match — new
`test_tsutil` passes 506 assertions in 32 cases. Full `ctest` is 127/127.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen merged PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517#discussion_r3751090391
##########
include/tsutil/Regex.h:
##########
@@ -68,7 +68,12 @@ class RegexMatches
/** Get the match at the given index.
*
- * @return The match at the given index.
+ * An index the match did not populate, and a group that did not participate in the match, both
+ * yield an empty view. The result never has a null data(), so it can be handed straight to
+ * memcpy(), std::string::append() and "%.*s". Use get_ovector_pointer() to tell a group that did
+ * not participate from one that matched an empty string.
Review Comment:
Outdated
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517#discussion_r3738318086
##########
include/tsutil/Regex.h:
##########
@@ -68,7 +68,12 @@ class RegexMatches
/** Get the match at the given index.
*
- * @return The match at the given index.
+ * An index the match did not populate, and a group that did not participate in the match, both
+ * yield an empty view. The result never has a null data(), so it can be handed straight to
+ * memcpy(), std::string::append() and "%.*s". Use get_ovector_pointer() to tell a group that did
+ * not participate from one that matched an empty string.
Review Comment:
The doc suggests using get_ovector_pointer() to distinguish a non-participating group from an empty-string match, but that only works for indices the match actually populated (index < size()). For index >= size(), the ovector entry is undefined, so callers should use size() as the primary bound and only consult PCRE2_UNSET within that bound.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517#issuecomment-5243936425
Cherry-picked to the 10.2.x branch as fc507f537d4e579bfd80e1b67d1dcde7a5b1c31c for the 10.2.0 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517#issuecomment-5242621109
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bound RegexMatches::operator[] by what the match populated [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13517:
URL: https://github.com/apache/trafficserver/pull/13517#issuecomment-5242858316
The diagnosis is right and the fix is in the correct place. A few things I verified that seem worth recording, including one that makes this more than a contract cleanup.
**The bound really was wrong.** Master checks `index >= pcre2_get_ovector_count(...)`, which is the *allocated* pair count — `DEFAULT_MATCHES` is 10 and the storage is a raw `char _buffer[24 + 96 + 28 * DEFAULT_MATCHES]` member with no initializer. So an index between the highest participating group and 10 passes the check and builds a view from whatever was in that buffer. Bounding by `_size` is the right fix because `_size` is what `pcre2_match()` reported it populated.
**The `rc == 0` case is why `_size <= 0` is safe, and it is worth being explicit about.** My first concern was that PCRE2 returns 0 for "match succeeded but the ovector was too small", with group 0 still valid — a naive `_size <= 0` guard would then return `""` for `matches[0]` on any pattern with more than 10 groups, which master handles correctly. That does not happen here, because `Regex::exec()` normalizes it first:
```cpp
if (rc == 0) {
matches._size = pcre2_get_ovector_count(RegexMatches::_MatchData::get(matches._match_data));
}
```
and in that case PCRE2 has filled every pair, so the allocated count *is* the populated count. After normalization `_size <= 0` can only mean a genuine error. Correct, but it depends on a normalization several dozen lines away in another function — a sentence in the comment pointing at it would save the next reader the same detour.
**A second improvement the description doesn't claim.** When `RE_FULL_MATCH` is requested and the match does not consume the whole subject, `exec()` sets `matches._size = PCRE2_ERROR_NOMATCH`. Under the new bound that makes a subsequent `matches[0]` return `""`; on master it still returns a live view over a match the caller was told to reject. Worth mentioning, since it is a behavior change beyond the stale-memory case.
**The `""`-instead-of-default-constructed part fixes something live, not just theoretical.** The description says this is a contract fix with nothing in tree able to reach it. That is true of the out-of-range read, but the null `data()` is reachable today via #13441. `plugins/prefetch/pattern.cc` guards up front with `if (_tokens[i] >= matchCount)`, so `replIndex < matchCount` holds — and #13441 returns a default-constructed `std::string_view` for a group *inside* that range that did not participate, whose `data()` is null (its own unit test asserts `matches[1].data() == nullptr`). `replace()` then does:
```cpp
PrefetchDebug("replacing '%s' with '%.*s'", src.c_str(), static_cast<int>(dst.length()), dst.data());
result.append(dst.data(), dst.length());
```
so a pattern with an optional group preceding a participating one — `/(v\d+/)?(.*-)(\d+)$/$1$2{$3+1}/` against a request without the prefix — passes a null pointer to `%.*s` and to `append(const char *, size_t)`. Benign on our libcs, undefined either way.
I raised exactly this on #13352 and suggested that PR normalize the view locally. Fixing it here instead is clearly the better place, since it covers all four call sites at once rather than one plugin. Worth a note on #13352 so the local guard isn't added redundantly — I'll leave that there.
**The no-end-offset-check rationale holds.** I checked that neither `PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK` nor `pcre2_set_compile_extra_options` appears anywhere in the tree, so the `\K`-in-lookaround escape hatch is genuinely unreachable here rather than merely unlikely.
One nit: the cachekey citation is off by a line. `plugins/cachekey/pattern.cc:271` is `String src(_replacement, _tokenOffset[i], 2);`, which does not involve `matches`; the `std::string` construction from a possibly-null pointer is the next line, `String dst(capture.data(), capture.length());`. The other three citations land exactly.
Nothing blocking.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org