Return an empty view for a non-participating capture group - #13441
Conversation
RegexMatches::operator[] only checked the index against the ovector count. A group that does not participate in the match has unset offsets, and an optional group that precedes a participating one is still within that count, so the check passes and the subject pointer is advanced by PCRE2_UNSET. The resulting view has length zero, so callers see an empty string today, but the pointer is invalid.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes RegexMatches::operator[] to safely handle non-participating (unset) capture groups by returning an empty std::string_view instead of producing an invalid pointer, and adds a regression test for the specific PCRE2 ovector behavior.
Changes:
- Return a default/empty
std::string_viewwhen a capture group’s start offset isPCRE2_UNSET. - Add a unit test covering an optional group that precedes a participating group (
(a)?(b)on"b").
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tsutil/Regex.cc | Adds a guard for unset ovector offsets to avoid invalid pointer arithmetic in RegexMatches::operator[]. |
| src/tsutil/unit_tests/test_Regex.cc | Adds a regression test validating the new behavior for non-participating capture groups. |
| // A group that did not participate in the match has an unset offset. This happens for an optional | ||
| // group that precedes a participating one, so a valid index is not enough to guarantee an offset. | ||
| if (PCRE2_UNSET == ovector[2 * index]) { | ||
| return std::string_view(); | ||
| } | ||
|
|
||
| return std::string_view(_subject.data() + ovector[2 * index], ovector[2 * index + 1] - ovector[2 * index]); |
There was a problem hiding this comment.
Approving as long as you are ok with this
There was a problem hiding this comment.
Neither case is reachable.
Unset offsets are set as a pair — pcre2api: "both values in the offset pairs corresponding to unused groups are set to PCRE2_UNSET." Checking the start is sufficient.
end < start requires \K inside a lookaround, which PCRE2 has rejected at compile time since 10.38 (\K is not allowed in lookarounds) unless PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK is set; ATS never calls pcre2_set_compile_extra_options. With the option forced on anyway, PCRE2 clamps rather than inverting — foo(?=bar\K)bar on "foobar" yields [6,6), not [6,3).
There is a real gap next to this one, though. The index is checked against the allocated ovector count, but pcre2_match() writes only up to the highest participating group, so an index past the pattern's groups reads the uninitialized tail of _buffer — the UNSET check doesn't catch it. Follow-up: #13517.
| // pcre2_match() returns one past the highest participating group, so an earlier optional group | ||
| // that did not participate is still within that count. Its offsets are unset. | ||
| Regex r; | ||
| REQUIRE(r.compile("(a)?(b)") == true); |
There was a problem hiding this comment.
Keeping == true for consistency with the rest of the file, which uses it 110 times and the bare form zero times. Dropping it here would make this the only section that differs.
|
[approve ci autest] |
RegexMatches::operator[] only checked the index against the ovector count. A group that does not participate in the match has unset offsets, and an optional group that precedes a participating one is still within that count, so the check passes and the subject pointer is advanced by PCRE2_UNSET. The resulting view has length zero, so callers see an empty string today, but the pointer is invalid. (cherry picked from commit e3bd689)
|
Cherry-picked to the 10.2.x branch as a378cca for the 10.2.0 release. |
A capture group that does not participate in a match has unset offsets.
RegexMatches::operator[]only compared the index against the ovector count, which is not sufficient:pcre2_match()returns one past the highest participating group, so an optional group that precedes a participating one is inside that count with its offsets unset.For pattern
(a)?(b)on subjectb, the match count is 3 and group 1 isPCRE2_UNSET. The index check passes and the subject pointer is advanced byPCRE2_UNSET.The resulting view has length zero, because the length is computed as
end - startand both are unset, so callers see an empty string and nothing observably breaks today. The pointer itself is invalid, which is what this fixes.operator[]now returns a defaultstd::string_viewfor a group that did not participate.Found while reviewing #13352, which works around the same underlying behavior in the prefetch plugin. That plugin guards the trailing-optional-group case; this is the general fix, and other callers index the ovector the same way (
plugins/regex_remap/regex_remap.cc,plugins/cachekey/pattern.cc,plugins/regex_revalidate/regex_revalidate.cc).Tests
New section in
test_Regex.cccovering a non-participating group before a participating one. It fails on master (the returned pointer is non-null) and passes with this change. Fulltest_tsutilsuite passes: 458 assertions, 28 cases.