Bound RegexMatches::operator[] by what the match populated - #13517
Open
moonchen wants to merge 2 commits into
Open
Bound RegexMatches::operator[] by what the match populated#13517moonchen wants to merge 2 commits into
moonchen wants to merge 2 commits into
Conversation
The ovector holds a fixed number of pairs regardless of the pattern, but pcre2_match() writes no further than the highest participating group. Indexing past that read the uninitialized remainder of the internal buffer and built a view from it, so checking PCRE2_UNSET was not by itself enough. Bound the index by the match size instead, and return an empty view built from "" rather than a default-constructed one. Callers pass the result straight to memcpy(), std::string::append() and "%.*s", none of which accept a null pointer.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the tsutil::RegexMatches contract to prevent RegexMatches::operator[] from constructing std::string_view values from uninitialized/undefined PCRE2 ovector entries, and ensures empty results return a non-null data() pointer to avoid UB in common callers.
Changes:
- Update
RegexMatches::operator[]to bound indexing by what the lastpcre2_match()call actually populated (_size), and return""for empty cases (out-of-range or non-participating group). - Expand unit tests to cover: trailing optional groups beyond the populated match count, indexing past populated groups but within allocated ovector capacity, and indexing after a failed match.
- Update public header documentation to describe the new empty-result behavior (non-null
data()).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/tsutil/Regex.cc |
Bounds operator[] by populated match size and returns non-null empty views for out-of-range / unset captures. |
include/tsutil/Regex.h |
Documents the updated operator[] contract and non-null empty return value. |
src/tsutil/unit_tests/test_Regex.cc |
Adds/updates regression coverage for non-participating groups, indices past populated groups, and post-failure indexing. |
Comment on lines
+71
to
+74
| * 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. |
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.
Problem
RegexMatchesallocates 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 anuninitialized member.
operator[]checked the index against the allocated pair count. So an index past thepattern's groups passed the check and built a
string_viewout of that leftover memory:a bad pointer with a meaningless length.
The
PCRE2_UNSETcheck added in #13441 doesn't cover this. It catches a group the matchreached 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 failedexec()hands back a view over garbage.Nothing in tree can reach this today:
regex_remaprejects a$nabove the pattern'scapture count at config load, prefetch and cachekey bound the index by the match count,
and
SSLSNIConfigiterates tomatches.size(). This fixes the contract, not a live bug.Fix
Bound the index by the match size — what
pcre2_match()actually populated — rather thanthe allocated pair count. Keep the
PCRE2_UNSETcheck for a group inside that range thatdidn't participate, which is the case #13441 was about.
Also return
""rather than a default-constructedstd::string_viewfor both emptycases, so the result never has a null
data(). Callers pass it straight into functionsthat don't accept a null pointer even at zero length:
plugins/regex_remap/regex_remap.cc:540—memcpy()plugins/cachekey/pattern.cc:271—std::stringctorplugins/prefetch/pattern.cc:267—std::string::append()plugins/experimental/access_control/pattern.cc:287—std::stringctorget_ovector_pointer()is still available to tell a group that didn't participate fromone 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_UNSETtogether, so testing thestart is enough.
end < startwould need\Kinside a lookaround, which PCRE2 has rejected at compiletime since 10.38 unless
PCRE2_EXTRA_ALLOW_LOOKAROUND_BSKis 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:test_tsutilpasses 506 assertions in 32 cases. Fullctestis 127/127.