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