zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r3858309974
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,27 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far, hence the cheap
bail-out.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding only shrinks, so it's safe in place; the +1 is room for the NUL
it appends.
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1,
&written) == TS_SUCCESS) {
+ s.resize(pos + written);
+ } else {
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
Fixed in f687068a67.
The write itself is actually well-defined in C++20 — `[string.access]` says
`operator[](size())` returns a reference to a `charT` object where *modifying
it to any value other than `charT()`* is UB, so storing `'\0'` there is
permitted, and `data()` is guaranteed to alias `&operator[](i)` for `i` in `[0,
size()]`.
That said, you're right that it's reachable at `pos+len` (an invalid escape
consumes nothing, so `data_written == len`), and relying on that guarantee
isn't worth the reader's time. `normalize()` now grows the string by one
scratch byte for the call and resizes it away afterwards, so nothing depends on
the terminator slot.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -408,6 +409,16 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType |
None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ # An unresolved block comes back verbatim, so re-attaching mods would
duplicate them.
+ if mods and expr != stripped:
+ expr = f"{expr} with {','.join(mods)}"
+
Review Comment:
Good catch, fixed in f687068a67 — this one was a real bug, and my comment on
the guard was wrong.
Since `_percent_to_ident_or_func()` is handed the *stripped* block, a
verbatim return comes back without the mods, so re-attaching could never have
duplicated them; it just dropped them. `%{READ_RESPONSE_HDR_HOOK [NORM]}`
reversed to `%{READ_RESPONSE_HDR_HOOK}`.
Now the mods go back inside the braces when the block has no DSL equivalent,
and into a `with` clause when it does. Added regression tests for both paths in
`tests/test_coverage.py`.
--
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]
