Copilot commented on code in PR #13424:
URL: https://github.com/apache/trafficserver/pull/13424#discussion_r4264052294
##########
plugins/header_rewrite/operators.cc:
##########
@@ -532,6 +533,70 @@ OperatorRMDestination::exec(const Resources &res) const
return true;
}
+// OperatorSortDestination
+void
+OperatorSortDestination::initialize(Parser &p)
+{
+ Operator::initialize(p);
+
+ _url_qual = parse_url_qualifier(p.get_arg());
+
+ require_resources(RSRC_CLIENT_REQUEST_HEADERS);
+ require_resources(RSRC_SERVER_REQUEST_HEADERS);
+}
+
+bool
+OperatorSortDestination::exec(const Resources &res) const
+{
+ if (res._rri || (res.bufp && res.hdr_loc)) {
+ TSMBuffer bufp;
+ TSMLoc url_m_loc;
+
+ // Determine which TSMBuffer and TSMLoc to use
+ if (res._rri && !res.changed_url) {
+ bufp = res._rri->requestBufp;
+ url_m_loc = res._rri->requestUrl;
+ } else {
+ bufp = res.bufp;
+ if (TSHttpHdrUrlGet(res.bufp, res.hdr_loc, &url_m_loc) != TS_SUCCESS) {
+ Dbg(pi_dbg_ctl, "TSHttpHdrUrlGet was unable to return the url m_loc");
+ return true;
+ }
Review Comment:
`TSHttpHdrUrlGet` returns a `TSMLoc` that (in the TS API) must be released
with `TSHandleMLocRelease` when obtained from an HTTP header. In this hunk,
`url_m_loc` is never released, which can leak per-transaction resources. Track
whether `url_m_loc` was acquired via `TSHttpHdrUrlGet` and release it on all
exit paths (including early returns / error branches), e.g., via a small scope
guard/RAII helper.
##########
plugins/header_rewrite/operators.cc:
##########
@@ -532,6 +533,70 @@ OperatorRMDestination::exec(const Resources &res) const
return true;
}
+// OperatorSortDestination
+void
+OperatorSortDestination::initialize(Parser &p)
+{
+ Operator::initialize(p);
+
+ _url_qual = parse_url_qualifier(p.get_arg());
+
+ require_resources(RSRC_CLIENT_REQUEST_HEADERS);
+ require_resources(RSRC_SERVER_REQUEST_HEADERS);
+}
+
+bool
+OperatorSortDestination::exec(const Resources &res) const
+{
+ if (res._rri || (res.bufp && res.hdr_loc)) {
+ TSMBuffer bufp;
+ TSMLoc url_m_loc;
+
+ // Determine which TSMBuffer and TSMLoc to use
+ if (res._rri && !res.changed_url) {
+ bufp = res._rri->requestBufp;
+ url_m_loc = res._rri->requestUrl;
+ } else {
+ bufp = res.bufp;
+ if (TSHttpHdrUrlGet(res.bufp, res.hdr_loc, &url_m_loc) != TS_SUCCESS) {
+ Dbg(pi_dbg_ctl, "TSHttpHdrUrlGet was unable to return the url m_loc");
+ return true;
+ }
+ }
+
+ switch (_url_qual) {
+ case URL_QUAL_QUERY: {
+ int q_len = 0;
+ const char *q_ptr = TSUrlHttpQueryGet(bufp, url_m_loc, &q_len);
+ std::string_view query = q_len > 0 ? std::string_view(q_ptr,
static_cast<size_t>(q_len)) : std::string_view();
+
+ if (is_query_sorted(query)) {
+ Dbg(pi_dbg_ctl, "OperatorSortDestination::exec() QUERY already sorted,
leaving it unchanged");
+ break;
+ }
+
+ std::string sorted = sort_query(query);
+
+ if (TSUrlHttpQuerySet(bufp, url_m_loc, sorted.c_str(), sorted.size()) !=
TS_SUCCESS) {
+ Dbg(pi_dbg_ctl, "OperatorSortDestination::exec() unable to set QUERY");
+ break;
+ }
+ const_cast<Resources &>(res).changed_url = true;
+ res.reset_query_cache();
+ Dbg(pi_dbg_ctl, "OperatorSortDestination::exec() rewrote QUERY to
\"%s\"", sorted.c_str());
+ break;
+ }
+ default:
+ Dbg(pi_dbg_ctl, "Sort destination %i has no handler", _url_qual);
+ break;
+ }
+ } else {
+ Dbg(pi_dbg_ctl, "OperatorSortDestination::exec() unable to continue due to
missing bufp=%p or hdr_loc=%p, rri=%p!", res.bufp,
+ res.hdr_loc, res._rri);
+ }
Review Comment:
The top-level guard allows entry when `res._rri` is set even if
`res.bufp/res.hdr_loc` are not, but the else-branch unconditionally
dereferences `res.bufp`/`res.hdr_loc` (it’s taken whenever `res.changed_url` is
true). This can lead to calling `TSHttpHdrUrlGet` with null/invalid handles.
Restructure the selection logic so the `TSHttpHdrUrlGet` path is only taken
when `(res.bufp && res.hdr_loc)` is true (or split into two explicit guarded
branches: (1) `_rri && !changed_url`, (2) `bufp && hdr_loc`).
##########
plugins/header_rewrite/operators.cc:
##########
@@ -532,6 +533,70 @@ OperatorRMDestination::exec(const Resources &res) const
return true;
}
+// OperatorSortDestination
+void
+OperatorSortDestination::initialize(Parser &p)
+{
+ Operator::initialize(p);
+
+ _url_qual = parse_url_qualifier(p.get_arg());
+
+ require_resources(RSRC_CLIENT_REQUEST_HEADERS);
+ require_resources(RSRC_SERVER_REQUEST_HEADERS);
+}
+
+bool
+OperatorSortDestination::exec(const Resources &res) const
+{
+ if (res._rri || (res.bufp && res.hdr_loc)) {
+ TSMBuffer bufp;
+ TSMLoc url_m_loc;
+
+ // Determine which TSMBuffer and TSMLoc to use
+ if (res._rri && !res.changed_url) {
+ bufp = res._rri->requestBufp;
+ url_m_loc = res._rri->requestUrl;
+ } else {
+ bufp = res.bufp;
+ if (TSHttpHdrUrlGet(res.bufp, res.hdr_loc, &url_m_loc) != TS_SUCCESS) {
+ Dbg(pi_dbg_ctl, "TSHttpHdrUrlGet was unable to return the url m_loc");
+ return true;
+ }
+ }
+
+ switch (_url_qual) {
+ case URL_QUAL_QUERY: {
+ int q_len = 0;
+ const char *q_ptr = TSUrlHttpQueryGet(bufp, url_m_loc, &q_len);
+ std::string_view query = q_len > 0 ? std::string_view(q_ptr,
static_cast<size_t>(q_len)) : std::string_view();
+
+ if (is_query_sorted(query)) {
+ Dbg(pi_dbg_ctl, "OperatorSortDestination::exec() QUERY already sorted,
leaving it unchanged");
+ break;
+ }
+
+ std::string sorted = sort_query(query);
+
+ if (TSUrlHttpQuerySet(bufp, url_m_loc, sorted.c_str(), sorted.size()) !=
TS_SUCCESS) {
Review Comment:
`TSUrlHttpQuerySet` typically takes the length as an `int`. Passing
`sorted.size()` (a `size_t`) relies on an implicit narrowing conversion and can
trigger compiler warnings (and is theoretically unsafe for very large inputs).
Prefer an explicit cast to the TS API’s expected length type (and/or
clamp/check) before calling.
--
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]
