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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
