Remove unnecessary copies flagged by static analysis

[bryancall]

Static analysis flagged a set of unnecessary std::string copies. This fixes the ones that are real and leaves the ones that are not.

What changed

std::move on a local's last use in plugins/header_rewrite/parser.cc, plugins/cachekey/pattern.cc, and plugins/experimental/rate_limit/limiter.h, where a local std::string (or an element of a local vector that is about to be destroyed) was copied into a container or into a by-value parameter.

8 changes across 3 files.

Why these and not the others

Every site was checked individually rather than applied in bulk, and most of the reported findings were left alone.

The moved-from objects are never read again. In parser.cc the tokens vector is a by-value parameter (the caller deliberately passes a copy), so moving out of its elements cannot be observed by the caller. In limiter.h the Dbg() call reads prefix and tag before they are moved into initializeMetrics(), whose tag and prefix parameters are both by-value std::string.

One line in pattern.cc was deliberately left as a copy. Pattern::process() runs captures.begin() + 1 on a vector that Pattern::capture() can return empty, so the loop body reads past the end. A copy only reads out of bounds there; a move would also write, so this stays a copy until the underlying bug is fixed. That is filed as #13638.

Findings in test helpers were skipped: making a test copy one fewer string is not worth the churn. Findings where the "copy" is a std::string_view, or where the expression returns by value so a const-reference bind would remove no copy at all, were skipped as false positives.

The auto to auto const & conversions were dropped

An earlier revision of this branch also converted five auto binds to auto const & in src/proxy/http/remap/NextHopSelectionStrategy.cc and NextHopConsistentHash.cc, where YAML::Node::Scalar() returns const std::string &. Those five were reverted in 1f50277 and are not in this diff.

GCC's -Wdangling-reference could not prove the reference outlives the temporary Node handle that operator[] returns, and the tree builds with -Werror. The lifetime argument for keeping them was that Scalar() returns a reference into detail::node storage owned by the document's shared memory holder rather than into the handle, and that the Map & parameter keeps that holder alive. That reasoning may well be right, but it is exactly the kind of claim a warning exists to make someone re-check, and it is not worth suppressing a diagnostic to save a handful of string copies on a config-parsing path. They can be revisited on their own.

Big parameter passed by value was not addressed

A larger group of those findings on this same code was deliberately left alone. They are almost entirely ConfigContext and YAML::Node, which are reference-counted handles that a size-based heuristic flags by sizeof. ConfigContext documents at its declaration that copies are intentional and that move is suppressed so std::move silently copies, which execute_reload() depends on. const YAML::Node & would also change operator[] semantics, since the const overload does not create missing keys. Converting them would risk a silent config-parsing change for no measurable gain on a path that runs a handful of times per reload.

Testing

Builds clean with experimental plugins enabled. All 166 unit tests pass.

[Copilot]

Pull request overview

This PR targets static-analysis findings about unnecessary std::string copies by moving from locals at their final use in several ATS plugins.

Changes:

• Move std::string values into _mods, _op, and _arg in header_rewrite preprocessing when the source tokens are no longer needed.
• Move captured/replaced strings into the output vector in the cachekey pattern processor.
• Move prefix/tag strings into RateLimiter::initializeMetrics() to avoid by-value copies.

Reviewed changes

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

File	Description
plugins/header_rewrite/parser.cc	Uses std::move when storing token-derived strings into parser members/containers.
plugins/experimental/rate_limit/limiter.h	Moves local prefix/tag into initializeMetrics() call to avoid copies.
plugins/cachekey/pattern.cc	Moves locally-built capture/replacement strings into the result vector.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cmcfarlen]

Checked each of the moves for reads-after-move. The mechanical work is right, including the two that look wrong at a glance:

• parser.cc — _mods.push_back(std::move(t)) sits inside while (getline(iss, t, ',')), which is the shape that usually is a bug. It is safe here because std::getline is specified to call str.erase() before appending and a moved-from std::string is a valid object, so the next iteration re-initializes it. The std::find duplicate check runs before the move.
• parser.cc — _op = std::move(tokens[0]) followed by _arg = std::move(tokens[1]) is safe because moving elements does not change size(), and the subsequent loop starts at begin() + 2. tokens is a by-value parameter with a comment saying the caller passes a copy deliberately, so nothing escapes to the caller.
• pattern.cc — element, dst, and the local captures vector are all at last use, and CacheKeyDebug reads dst before the move.

Two things I would like addressed before this lands.

pattern.cc:156 — the move escalates an existing out-of-bounds read into an out-of-bounds write

Regex::exec returns pcre2_match's rc verbatim, and rc == 0 means the ovector was too small (src/tsutil/Regex.cc:506-515). It fixes up matches._size from pcre2_get_ovector_count, but the return value stays 0. Pattern::capture uses a default-constructed RegexMatches, which is DEFAULT_MATCHES = 10 slots (include/tsutil/Regex.h:82), so any cachekey pattern with more than about nine capture groups reaches that path.

Pattern::capture then loops on the returned matchCount rather than matches.size(), so it pushes nothing and returns true with an empty vector. Pattern::process sees size() != 1 and runs:

StringVector::iterator it = captures.begin() + 1;   // UB: begin() + 1 on an empty vector
for (; it != captures.end(); it++) {
  result.push_back(std::move(*it));                  // was push_back(*it)
}

Before this patch that is a garbage read past the end. With std::move it becomes a write: the out-of-bounds std::string is mutated to empty and whatever pointer was read there is freed. Same underlying bug, materially worse failure mode, reachable from a user's cachekey configuration.

The underlying fix is for capture() to loop on matches.size() instead of the return value, which also stops silently dropping captures on a successful match, and for process() to guard the empty case rather than trusting begin() + 1. If you would rather keep this PR purely mechanical, reverting just this one line to push_back(*it) and filing the capture() bug separately is fine too — I mainly do not want the move to ship on top of the existing UB.

limiter.h:228 — now conflicts with master

This hunk had the arguments swapped relative to initializeMetrics(uint type, std::string tag, std::string prefix). #13601 landed in the meantime (merged as c9e9bfe) and fixed exactly that line, so master now reads:

initializeMetrics(RATE_LIMITER_TYPE_SNI, tag, prefix);

A rebase is needed, and the moves should come back as std::move(tag), std::move(prefix) — both parameters are by-value std::string, so they are real wins, just on the corrected argument order.

Smaller items

• parser.cc:196 — after tokens.pop_back() consumes a flags-only line (a line consisting solely of [L], say), tokens is empty and the next statement is tokens[0].substr(0, 2). Pre-existing, and adjacent to a hunk here, so an if (tokens.empty()) guard after the pop is cheap if you want it; otherwise a follow-up is fine.
• The description is stale. It says "15 changes across 5 files" and describes the auto to auto const & conversions in NextHopSelectionStrategy.cc and NextHopConsistentHash.cc, but commit 1f50277 reverted all five of those, so the diff is three files. More confusingly, the rationale still argues at length that the const-reference binds do not dangle because Node::Scalar() returns a reference into detail::node storage, while that commit's own message says GCC's -Wdangling-reference could not prove it and the tree builds with -Werror. The commit message is the better analysis; please update the description so the record does not contradict itself.

Everything else in the diff is good, and CI is green across all fifteen checks. The pattern.cc line is the only real blocker.

[bryancall]

Thanks, this was a careful read. I walked each link of the pattern.cc chain against the source before acting on it, and every step holds. Point by point:

pattern.cc:156 — reverted to a copy, bug filed separately

Took your second option. result.push_back(std::move(*it)) is back to result.push_back(*it), and the underlying bug is #13638.

Everything you described checks out:

• Regex::exec sets matches._size = rc, repairs _size from pcre2_get_ovector_count() when rc == 0, then returns rc, so the return value stays 0 while _size is fixed (src/tsutil/Regex.cc:505-515).
• RegexMatches defaults to DEFAULT_MATCHES = 10 (include/tsutil/Regex.h:82).
• Pattern::capture loops for (int i = 0; i < matchCount; i++) on the return value, treats only a negative as failure, so matchCount == 0 returns true having pushed nothing.
• Pattern::process sees a size that is not 1, takes the else branch, and runs captures.begin() + 1.

One thing worth adding: on an empty vector the loop is not bounded at one element either. begin() and end() are equal, so begin() + 1 is already past end() and the it != captures.end() condition never becomes true going forward. It keeps walking.

I agree the real fix is what you described, capture() looping on matches.size() instead of the return value plus an empty guard in process(), and that it does not belong in a mechanical cleanup PR. #13638 has the full chain and both proposed changes.

limiter.h:228 — merged master, moves re-applied on the corrected order

Confirmed #13601 as c9e9bfe71e, and confirmed the declaration is initializeMetrics(uint type, std::string tag, std::string prefix) at limiter.h:191. The line now reads:

initializeMetrics(RATE_LIMITER_TYPE_SNI, std::move(tag), std::move(prefix));

I merged master into the branch rather than rebasing, so no force-push. That keeps your "changes since your last review" diff intact. Say the word if you would rather see a linear history and I will rebase, though the squash-merge collapses it either way.

parser.cc:196 — filed as a follow-up

#13639. Verified: a line whose only token is a flags section gives tokens.size() == 1, the pop_back() empties the vector, and the next statement is tokens[0].substr(0, 2). Keeping the guard out of this PR so the diff stays mechanical.

Description

Rewritten. It now says 8 changes across 3 files, and the section on the NextHop files states that the five conversions were reverted in 1f50277, that GCC's -Wdangling-reference could not prove the lifetime, and that the tree builds with -Werror. The old text arguing the binds could not dangle is gone. You are right that the commit message was the better analysis; the description was written before the revert and never caught up.

Both Copilot threads are replied to and resolved. All four formatters are clean, and the three affected plugins build locally.

[Copilot]

🔵 Needs a closer look

The PR description claims additional changes (and changed files) that are not present in the current diff/branch contents, so the description or the missing commits/files need to be reconciled before approval.

Review details

Suppressed comments (1)

plugins/header_rewrite/parser.cc:245

• PR description mentions additional copy-elimination changes in src/proxy/http/remap/NextHopSelectionStrategy.cc and NextHopConsistentHash.cc (and "15 changes across 5 files"), but those changes do not appear in the current diff/branch contents; please either update the PR description to match what’s actually included or add the missing commits/files so reviewers can validate them.

    _op = std::move(tokens[0]);
    if (tokens.size() > 1) {
      _arg = std::move(tokens[1]);

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite