bryancall commented on code in PR #13591:
URL: https://github.com/apache/trafficserver/pull/13591#discussion_r4067918982
##########
plugins/header_rewrite/parser.cc:
##########
@@ -185,13 +185,20 @@ Parser::preprocess(std::vector<std::string> tokens)
// This produces an error, but it's not fatal for load / reload.
ToDo: ATS v11 fix.
TSError("[%s] Duplicate modifier: %s", PLUGIN_NAME, t.c_str());
} else {
- _mods.push_back(t);
+ _mods.push_back(std::move(t));
}
}
} else {
- _mods.push_back(m);
+ _mods.push_back(std::move(m));
}
tokens.pop_back(); // consume it, so we don't concatenate it into the
value
+ if (tokens.empty()) {
+ // Nothing is left to parse, and the code below indexes tokens[0]
+ // unconditionally. Reading it would touch the element pop_back()
just
+ // destroyed, and _op would then take ownership of freed memory.
+ TSError("[%s] modifiers with no condition or operator to apply them
to", PLUGIN_NAME);
Review Comment:
Documented rather than split, because the guards are not a separate change.
They are what makes one of the moves in this diff safe.
`preprocess()` checks `tokens.size() > 0` before consuming a trailing
modifier group, then indexes `tokens[0]` unconditionally about fifteen lines
further down. Two things in between can empty the list. `tokens.pop_back()`
consumes a trailing `[...]` group, so a line that is nothing but a group
empties it. `tokens.erase(tokens.begin())` consumes the `cond` keyword, so a
bare `cond` line empties it.
`_op = tokens[0]` on an empty vector only reads it. `_op =
std::move(tokens[0])` would also write to it. Landing the move without the
guard would be worse than leaving the copy, so the two belong in the same
change.
That is the same call I made on the `pattern.cc` line, decided the other way
round. There the underlying problem is in `Pattern::process()`'s own iteration
and is not a two-line fix, so that line stays a copy and the bug is filed as
#13638. Here it is two `if (tokens.empty())` returns that match every other
malformed-line path in the function, so it is fixed in place.
Title and description now lead with it. I also dropped a stale paragraph
about `limiter.h`, which is no longer part of this diff.
--
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]
