prefetch: don't drop replacements for non-participating optional capture groups by moonchen · Pull Request #13352 · apache/trafficserver

moonchen · 2026-07-01T03:34:24Z

Problem

Pattern::replace() in the prefetch plugin rejects any replacement reference $N whose index is >= the value returned by the match. But that value is one past the highest capture group that participated in the match, not the number of groups the pattern defines. A trailing optional group such as (\?.*)? that does not participate — e.g. a request with no query string — lowers that count and makes a perfectly valid $N look out of range.

Concretely, with a remap like:

@plugin=prefetch.so @pparam=--fetch-path-pattern=/(.*-)(\d+)(\?.*)?$/$1{$2+1}$3/

every matching request without a query string logs:

ERROR: (prefetch) invalid reference in replacement string: $3

and the prefetch is silently dropped (the client is still served).

Fix

replace(): at match time, substitute an empty string for a group that did not participate instead of failing the whole replacement — the documented PCRE2 semantics for an unmatched group.
compile(): validate $N references once, at config-load time, against the pattern's actual capture-group count (Regex::get_capture_count()) — the correct place to catch a genuinely out-of-range reference such as $5 against a 3-group pattern, and a pattern defining more groups than can be captured ($0..$9).

Additional hardening

Treat an unusable --fetch-path-pattern, and invalid --fetch-count / --fetch-max / --fetch-overflow values, as configuration errors: the remap instance fails to load instead of silently running with prefetch disabled. On a running server, traffic_ctl config reload safely rejects the bad config and keeps the current one.
Skip an empty expanded path instead of scheduling a zero-length fetch (which BgFetch turns into a self-prefetch of the original request path).
Remove the now-dead Pattern::process() / Pattern::capture() helpers (no callers).

Note for reviewers: the "refuse to load on invalid config" behavior change is intentional and aligns with how --fetch-policy already behaves, but it is a behavior change — happy to split it into a separate PR if preferred.

Tests

Adds three autests under tests/gold_tests/pluginTest/prefetch/:

prefetch_optional_group — the non-participating optional-group case (fails on the pre-fix code).
prefetch_bad_pattern_refused — an over-limit (10-group) pattern makes ATS refuse to load the remap.
prefetch_empty_replacement — an empty-collapsing replacement is logged and skipped, not self-prefetched.

Built locally (macOS, PCRE2); all prefetch autests pass. Draft pending CI.

Copilot

Pull request overview

This PR fixes how the prefetch plugin expands $N references in --fetch-path-pattern replacements so that defined-but-non-participating optional capture groups expand to an empty string (PCRE2 semantics) instead of invalidating the entire replacement. It also tightens config-load validation for replacement references and several prefetch options, and adds AuTest coverage for the reported regressions.

Changes:

Validate $N replacement references at config-load time against the regex’s defined capture-group count, and expand non-participating optional groups to "" at match time.
Treat invalid prefetch configuration (bad pattern / bad numeric options / invalid overflow policy) as a remap-load failure rather than silently disabling prefetch.
Add gold tests covering optional-group non-participation, empty-collapsing replacements, and over-limit capture-group patterns.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.test.py	New autest verifying optional trailing group `$3` does not cause per-request replacement rejection.
tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.gold	Expected request log lines for the optional-group test.
tests/gold_tests/pluginTest/prefetch/prefetch_empty_replacement.test.py	New autest verifying empty-expanded paths are logged and skipped (no self-prefetch).
tests/gold_tests/pluginTest/prefetch/prefetch_bad_pattern_refused.test.py	New autest verifying over-limit capture-group patterns are refused at config load.
plugins/prefetch/plugin.cc	Skip scheduling when a replacement collapses to an empty expanded path.
plugins/prefetch/pattern.h	Remove now-dead helper declarations.
plugins/prefetch/pattern.cc	Adjust replacement expansion semantics; add config-load validation for `$N` references.
plugins/prefetch/configs.h	Change `setFetchOverflow` to return `bool` for validation.
plugins/prefetch/configs.cc	Enforce stricter option validation and fail remap instance creation on invalid config.

serrislew · 2026-07-21T23:33:56Z

+  if (nullptr == optarg || '\0' == *optarg) {
+    return false;
+  }
+  for (const char *p = optarg; '\0' != *p; ++p) {


nitpicky but does this need any range-checking?

Good catch.

Switched both --fetch-count and --fetch-max to std::from_chars() parsing directly into the destination unsigned, which reports out-of-range and trailing junk in the same call. Added prefetch_bad_count_refused to cover it; it fails against the previous code.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

plugins/prefetch/configs.cc:187

The parser rejects inputs like +10 and any leading whitespace (which some users may still think of as 'non-negative integers'), but the error message suggests those should be accepted. Consider tightening the wording to match the actual accepted format (e.g., 'expected an unsigned decimal integer without sign characters') or explicitly documenting/allowing a leading '+'.

      if (unsigned count = 0; parseUnsignedInt(optarg, count)) {
        setFetchCount(count);
      } else {
        PrefetchError("invalid --fetch-count '%s': expected a non-negative integer", optarg ? optarg : "");
        status = false;
      }

tests/gold_tests/pluginTest/prefetch/prefetch_optional_group.test.py:2

The file starts with two empty triple-quoted string literals; these create an empty module docstring/no-op string expression and add noise. Remove them, or replace with a single meaningful module docstring (you already have Test.Summary).

'''
'''

moonchen · 2026-08-07T16:22:54Z

  for (int i = 0; i < _tokenCount; i++) {
-    int              replIndex = _tokens[i];
-    std::string_view dst       = matches[replIndex];
+    int replIndex = _tokens[i];


Fixed. The view is now built from "" so data() is never null, and the append uses the std::string_view overload rather than the pointer/length pair, so neither the "%.*s" nor the append passes a null pointer.

serrislew · 2026-07-28T21:00:35Z

+                   * absent) would otherwise be scheduled with a zero-length path, which BgFetch skips --
+                   * leaving the original request path in place and prefetching the pristine URL itself.
+                   * Stop rather than issue that self-prefetch. */
+                  PrefetchError("prefetch pattern produced an empty path; check the fetch-path-pattern replacement");


If this is per transaction, will this flood our logs with this error?

Good catch, and worse than per transaction: whether the replacement collapses depends on the request, so for a misconfigured pattern it recurs indefinitely rather than being a one-time complaint at load.

PrefetchConfig::shouldReportEmptyPath() now reports it at error level once per remap instance and drops to the debug tag afterwards. A config reload builds a new instance, so a genuine misconfiguration stays visible after each reload.

serrislew · 2026-07-28T21:03:58Z

since validation behavior changes, does this need a doc update?

Pattern::replace() rejected any replacement reference $N whose index was >= the value returned by the match. But that value is one past the highest capture group that *participated* in the match, not the number of groups the pattern defines. A trailing optional group such as "(\?.*)?" that does not participate -- e.g. a request with no query string -- lowers that count and makes a valid $N look out of range, so every such request logged "invalid reference in replacement string: $N" and silently dropped the prefetch. Validate $N references once at config-load time against the pattern's actual capture-group count (Regex::get_capture_count()), and at match time substitute an empty string for a group that did not participate instead of failing the whole replacement -- the documented PCRE2 semantics for an unmatched group. Also treat an unusable fetch-path-pattern, and invalid --fetch-count / --fetch-max / --fetch-overflow values, as configuration errors so the remap rule is refused at load rather than silently running with prefetch disabled; skip an empty expanded path instead of self-prefetching the original request path; and remove the now-dead process()/capture() helpers. Adds autests for the non-participating-group fix, the rejected over-limit pattern, and the empty-replacement skip.

The digits-only check on --fetch-count and --fetch-max accepted any string of decimal digits. The value then went through strtoul() into a size_t and was assigned to an unsigned, so a value above UINT_MAX was silently truncated: --fetch-count=5000000000 became 705032704. Parse with std::from_chars() into the destination type instead. It reports an out-of-range value and a trailing non-digit, so both are configuration errors now, consistent with the other option values.

A group that did not participate yielded a default-constructed string_view, whose data() is null. Passing that to "%.*s" and to append(ptr, 0) is undefined even at zero length, so build the view from "" and append the view itself. Report an expanded path that collapsed to empty once per remap instance. The condition depends on the request, so at error level it repeated for every matching transaction. A config reload builds a new instance and reports again. Document that an invalid --fetch-count, --fetch-max, --fetch-overflow or --fetch-path-pattern makes the remap rule fail to load rather than loading with prefetch silently disabled.

moonchen · 2026-08-07T16:23:10Z

since validation behavior changes, does this need a doc update?

Yes, added. There is now a paragraph after the parameter reference in doc/admin-guide/plugins/prefetch.en.rst stating that an invalid parameter value is a configuration error: --fetch-count and --fetch-max must be decimal numbers that fit in an unsigned integer, --fetch-overflow must be 32 or 64, and --fetch-path-pattern must compile and may only reference capture groups the pattern defines. It also says the remap rule fails to load rather than loading with prefetch silently disabled, and that traffic_ctl config reload rejects such a configuration and keeps the running one.

Rebased onto master while here, which picked up #71d40c137c (Constrain prefetch relative paths). That reworks the --fetch-query relative-path branch, so it does not overlap the --fetch-path-pattern branch this PR touches. All nine prefetch autests pass locally, including the prefetch_query_path_traversal test that came with it.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

doc/admin-guide/plugins/prefetch.en.rst:300

The new configuration-validation note says --fetch-overflow must be 32 or 64, but the plugin documentation earlier (and the implementation) also supports bignum. The plugin-parameter list here also omits --fetch-overflow entirely, even though it’s referenced immediately below. Please align this section with the supported values and list the option among the parameters.

An invalid parameter value is a configuration error. ``--fetch-count`` and ``--fetch-max`` must be
decimal numbers that fit in an unsigned integer, ``--fetch-overflow`` must be ``32`` or ``64``, and
``--fetch-path-pattern`` must compile and may only reference capture groups that the pattern
defines. A remap rule with an invalid value fails to load, instead of loading with prefetch
silently disabled. ``traffic_ctl config reload`` rejects such a configuration and keeps the running

cmcfarlen · 2026-08-07T17:54:36Z

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:

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:

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:

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.

moonchen · 2026-08-10T15:54:02Z

Holding this until #13517 merges.

As it stands here, replace() still takes the matches[replIndex] branch for a non-participating group, which returns a null data() since #13441 landed. #13517 fixes that in RegexMatches::operator[] itself, after which this collapses to a plain matches[replIndex] with no guard needed. Rebasing once it's in.

…ure groups (#13352) Pattern::replace() rejected any $N whose index was >= the match's return value. That value is one past the highest capture group that *participated*, not the number of groups the pattern defines, so a trailing optional group such as "(\?.*)?" that did not participate made a valid $N look out of range. Every such request logged "invalid reference in replacement string" and silently dropped the prefetch. Validate $N once at config-load time against the pattern's actual capture-group count, and substitute an empty string at match time for a group that did not participate, per PCRE2 semantics. Also treat an unusable --fetch-path-pattern and invalid --fetch-count / --fetch-max / --fetch-overflow values as configuration errors, so the remap rule is refused at load rather than running with prefetch silently disabled. --fetch-count and --fetch-max now parse with std::from_chars(), which rejects a value above UINT_MAX instead of truncating it. Skip an empty expanded path rather than self- prefetching the original, and report that once per remap instance since the condition is request-dependent. Removes the now-dead Pattern::process() and Pattern::capture(). (cherry picked from commit adbd85d)

cmcfarlen · 2026-08-10T17:54:22Z

Cherry-picked to the 10.2.x branch as 2fe4afb for the 10.2.0 release.

Three late bug fixes on 10.2.x. All are fixes with no new configuration, metrics or API surface, so only the changelog and the commit/PR counts change.

* Add 10.2.0 changelog and release notes Generate CHANGELOG-10.2.0 from the 10.2.0 milestone and document the release in whats-new and upgrading. The connect retry change (#13102) is called out as a necessary incompatible change, since the retry limits were not previously applied according to origin state. * Address review: fix PR count and token_key markup The PR count was 655 before five stale milestone entries were dropped; the changelog has 650. Use :ts:cv: for proxy.config.quic.server.token_key.filename, which is documented on 10.2.x even though it is absent from master, where it was first checked. * Add late 10.2.x additions to changelog and release notes Picks up #13328 (shared-memory cache directory for fast restart) and #13418 (traffic_ctl cache clear). The shm directory gets its own section since it is a new opt-in feature with four new records and a traffic_ctl subcommand. * Add July 2026 security fixes to changelog and release notes The Release 2 security bundle (#13452) landed directly on 10.2.x without public PRs, so those commits never appear in a milestone. Source them from the commit range with the changelog tool's git-range mode and append them as bare subjects, matching how CHANGELOG-10.1.4 lists them. Link the advisory from whats-new for the CVE mapping. * Add #13352, #13517 and #13523 to the changelog Three late bug fixes on 10.2.x. All are fixes with no new configuration, metrics or API surface, so only the changelog and the commit/PR counts change.

bryancall assigned moonchen Jul 6, 2026

moonchen requested a review from serrislew July 18, 2026 17:46

moonchen added Plugins Bug labels Jul 19, 2026

moonchen added this to the 11.0.0 milestone Jul 19, 2026

moonchen added the prefetch prefetch plugin label Jul 19, 2026

moonchen added this to ATS v10.2.x Jul 19, 2026

moonchen marked this pull request as ready for review July 20, 2026 20:28

Copilot AI lite review requested due to automatic review settings July 20, 2026 20:28

Copilot started reviewing on behalf of moonchen July 20, 2026 20:29 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread plugins/prefetch/pattern.cc Outdated

serrislew reviewed Jul 21, 2026

View reviewed changes

moonchen mentioned this pull request Jul 28, 2026

Return an empty view for a non-participating capture group #13441

Merged

moonchen requested review from Copilot and serrislew July 28, 2026 15:37

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot started reviewing on behalf of moonchen July 28, 2026 18:08 View session

serrislew reviewed Jul 28, 2026

View reviewed changes

moonchen added 3 commits August 5, 2026 16:45

moonchen force-pushed the prefetch-optional-group-fix branch from 2bdf0d5 to 3f15e3c Compare August 7, 2026 16:21

Copilot AI review requested due to automatic review settings August 7, 2026 16:21

Copilot started reviewing on behalf of moonchen August 7, 2026 16:30 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

cmcfarlen approved these changes Aug 7, 2026

View reviewed changes

cmcfarlen mentioned this pull request Aug 10, 2026

Bound RegexMatches::operator[] by what the match populated #13517

Merged

moonchen merged commit adbd85d into apache:master Aug 10, 2026
15 checks passed

github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Aug 10, 2026

cmcfarlen mentioned this pull request Aug 10, 2026

Cherry-picks for 10.2.0 RC, round 5 (2026-08-10) #13526

Merged

cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Aug 10, 2026

cmcfarlen modified the milestones: 11.0.0, 10.2.0 Aug 10, 2026

Uh oh!

Conversation

moonchen commented Jul 1, 2026

Problem

Fix

Additional hardening

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

serrislew Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

moonchen Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

moonchen Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serrislew Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

moonchen Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

serrislew commented Jul 28, 2026

Uh oh!

moonchen commented Aug 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

cmcfarlen commented Aug 7, 2026

Uh oh!

moonchen commented Aug 10, 2026

Uh oh!

Uh oh!

cmcfarlen commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moonchen Jul 28, 2026 •

edited

Loading