Return an empty view for a non-participating capture group - #13441
Open
moonchen wants to merge 1 commit into
Open
Return an empty view for a non-participating capture group#13441moonchen wants to merge 1 commit into
moonchen wants to merge 1 commit into
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.
Contributor
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. |
Comment on lines
+209
to
215
| // 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]); |
| // 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); |
Contributor
Author
|
[approve ci autest] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.