cmcfarlen commented on PR #13352:
URL: https://github.com/apache/trafficserver/pull/13352#issuecomment-5220360869
The core diagnosis is right and I verified the pieces it rests on.
`PCRE2_INFO_CAPTURECOUNT` does report the groups the pattern *defines*
(excluding group 0), so validating `$N` against `Regex::get_capture_count()` at
compile time is the correct bound, and `_tokens[i] > captureCount` is the right
comparison — `$3` against a 3-group pattern is legal, `$4` is not. Moving that
check out of the per-match path into `compile()` is the real fix: the old code
was comparing a config-time constant against a per-request value, which is why
a request without a query string could invalidate a `$3` that is perfectly
valid for the pattern.
The hardening is solid too. `std::errc{} == ec && parsed == end` is the
right `from_chars()` idiom — it rejects trailing junk and out-of-range in one
call, which a `strtoul`-style check usually misses. And
`shouldReportEmptyPath()` via `_reportedEmptyPath.exchange(true, relaxed)`
correctly makes the empty-path complaint once per instance rather than per
transaction, which matters here precisely because whether the replacement
collapses is request-dependent.
**One thing I think is still open, and it is an ordering hazard with
#13441.**
The null-pointer guard covers only one of the two ways `dst` can end up
empty:
```cpp
std::string_view dst = (replIndex < matchCount) ? matches[replIndex] :
std::string_view{""};
```
The `""` protects the `replIndex >= matchCount` branch — the
trailing-optional-group case this PR is about. But your own PCRE2 analysis in
the thread above establishes the other case: for `(a)?(b)` on subject `b`,
`matchCount` is 3 and group 1 is `PCRE2_UNSET`, so `replIndex < matchCount`
holds and the *first* branch is taken.
Today that yields a garbage pointer with zero length. Once #13441 lands,
`RegexMatches::operator[]` returns `std::string_view()` for an unset group —
its unit test asserts `matches[1].data() == nullptr` explicitly. So after
#13441, this line hands a genuine `nullptr` to:
```cpp
PrefetchDebug("replacing '$%d' with '%.*s'", replIndex,
static_cast<int>(dst.length()), dst.data());
result.append(dst);
```
which is the exact `%.*s`-with-null concern raised at pattern.cc:186/198.
The reply there says the view is now built from `""` so `data()` is never null;
that holds for the branch it was aimed at, but not for this one. Benign on the
libcs we build against, still undefined, and #13441 turns it from "invalid
pointer" into "null pointer" — the case implementations are least likely to
tolerate.
It is reachable with an ordinary pattern, not just a contrived one: any
optional group that precedes a participating group, e.g.
`--fetch-path-pattern=/(v\d+/)?(.*-)(\d+)$/$1$2{$3+1}/` on a request without
the version prefix.
Normalizing regardless of which branch produced the view closes it and makes
the guard say what the comment already claims:
```cpp
std::string_view dst = (replIndex < matchCount) ? matches[replIndex] :
std::string_view{};
if (nullptr == dst.data()) {
dst = std::string_view{""};
}
```
That also means this PR stops depending on #13441's choice of return value
either way, which seems worth having given both are in flight — right now the
correctness of this line is coupled to a decision made in another PR.
Two smaller notes:
- Dropping `Pattern::process()` and `Pattern::capture()` is fine — I
confirmed no remaining callers. Worth keeping in the description since removing
public-looking plugin methods tends to raise eyebrows.
- The "refuse to load on invalid config" change is the right call and
matching `--fetch-policy`'s existing behavior is a good argument, but it is a
genuine behavior change: an operator whose `--fetch-count` was silently ignored
now fails the remap. You already flagged it and offered to split; I would keep
it here, since shipping the validation without the enforcement would leave the
bad config running. Just make sure it lands in a release note.
Nothing else from me — with the null normalization I would be happy with
this.
--
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]
