This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new bdb24cfd93 Fix set-status crash inside if/endif at remap time (#13052)
bdb24cfd93 is described below
commit bdb24cfd93e1c6e9a27a4d03b126121d085596da
Author: Mo Chen <[email protected]>
AuthorDate: Tue Jun 23 16:59:35 2026 -0500
Fix set-status crash inside if/endif at remap time (#13052)
---
include/proxy/hdrs/HTTP.h | 4 +--
plugins/header_rewrite/operators.cc | 18 +++++--------
plugins/header_rewrite/ruleset.cc | 4 +--
plugins/header_rewrite/statement.cc | 10 ++-----
plugins/header_rewrite/statement.h | 10 ++-----
src/proxy/hdrs/HTTP.cc | 4 +--
.../header_rewrite_bundle.replay.yaml | 31 ++++++++++++++++++++++
.../header_rewrite/rules/set_status_in_if.conf | 27 +++++++++++++++++++
8 files changed, 74 insertions(+), 34 deletions(-)
diff --git a/include/proxy/hdrs/HTTP.h b/include/proxy/hdrs/HTTP.h
index 104743e593..571bd0fdc5 100644
--- a/include/proxy/hdrs/HTTP.h
+++ b/include/proxy/hdrs/HTTP.h
@@ -1097,7 +1097,7 @@ inline void
HTTPHdr::status_set(HTTPStatus status)
{
ink_assert(valid());
- ink_assert(m_http->m_polarity == HTTPType::RESPONSE);
+ ink_release_assert(m_http->m_polarity == HTTPType::RESPONSE);
http_hdr_status_set(m_http, status);
}
@@ -1121,7 +1121,7 @@ inline void
HTTPHdr::reason_set(std::string_view value)
{
ink_assert(valid());
- ink_assert(m_http->m_polarity == HTTPType::RESPONSE);
+ ink_release_assert(m_http->m_polarity == HTTPType::RESPONSE);
http_hdr_reason_set(m_heap, m_http, value, true);
}
diff --git a/plugins/header_rewrite/operators.cc
b/plugins/header_rewrite/operators.cc
index e82b834468..133991228c 100644
--- a/plugins/header_rewrite/operators.cc
+++ b/plugins/header_rewrite/operators.cc
@@ -205,19 +205,13 @@ OperatorSetStatus::initialize_hooks()
bool
OperatorSetStatus::exec(const Resources &res) const
{
- switch (get_hook()) {
- case TS_HTTP_READ_RESPONSE_HDR_HOOK:
- case TS_HTTP_SEND_RESPONSE_HDR_HOOK:
- if (res.bufp && res.hdr_loc) {
- TSHttpHdrStatusSet(res.bufp, res.hdr_loc,
static_cast<TSHttpStatus>(_status.get_int_value()), res.state.txnp,
PLUGIN_NAME);
- if (_reason && _reason_len > 0) {
- TSHttpHdrReasonSet(res.bufp, res.hdr_loc, _reason, _reason_len);
- }
+ if (res.bufp && res.hdr_loc && TSHttpHdrTypeGet(res.bufp, res.hdr_loc) ==
TS_HTTP_TYPE_RESPONSE) {
+ TSHttpHdrStatusSet(res.bufp, res.hdr_loc,
static_cast<TSHttpStatus>(_status.get_int_value()), res.state.txnp,
PLUGIN_NAME);
+ if (_reason && _reason_len > 0) {
+ TSHttpHdrReasonSet(res.bufp, res.hdr_loc, _reason, _reason_len);
}
- break;
- default:
+ } else {
TSHttpTxnStatusSet(res.state.txnp,
static_cast<TSHttpStatus>(_status.get_int_value()), PLUGIN_NAME);
- break;
}
Dbg(pi_dbg_ctl, "OperatorSetStatus::exec() invoked with status=%d",
_status.get_int_value());
@@ -608,7 +602,7 @@ OperatorSetRedirect::exec(const Resources &res) const
const_cast<Resources &>(res).changed_url = true;
res._rri->redirect = 1;
} else {
- Dbg(pi_dbg_ctl, "OperatorSetRedirect::exec() hook=%d", int(get_hook()));
+ Dbg(pi_dbg_ctl, "OperatorSetRedirect::exec() redirect to %s",
value.c_str());
// Set the new status code and reason.
TSHttpStatus status = static_cast<TSHttpStatus>(_status.get_int_value());
TSHttpHdrStatusSet(res.bufp, res.hdr_loc, status, res.state.txnp,
PLUGIN_NAME);
diff --git a/plugins/header_rewrite/ruleset.cc
b/plugins/header_rewrite/ruleset.cc
index 39964fc68e..acbeafc72f 100644
--- a/plugins/header_rewrite/ruleset.cc
+++ b/plugins/header_rewrite/ruleset.cc
@@ -67,7 +67,7 @@ RuleSet::make_condition(Parser &p, const char *filename, int
lineno)
Dbg(pi_dbg_ctl, " Creating condition: %%{%s} with arg: %s",
p.get_op().c_str(), p.get_arg().c_str());
c->set_config_location(filename, lineno);
c->initialize(p);
- if (!c->set_hook(_hook)) {
+ if (!c->is_hook_valid(_hook)) {
delete c;
TSError("[%s] in %s:%d: can't use this condition in hook=%s: %%{%s} with
arg: %s", PLUGIN_NAME, filename, lineno,
TSHttpHookNameLookup(_hook), p.get_op().c_str(),
p.get_arg().c_str());
@@ -95,7 +95,7 @@ RuleSet::add_operator(Parser &p, const char *filename, int
lineno)
Dbg(pi_dbg_ctl, " Adding operator: %s(%s)=\"%s\"", p.get_op().c_str(),
p.get_arg().c_str(), p.get_value().c_str());
op->set_config_location(filename, lineno);
op->initialize(p);
- if (!op->set_hook(_hook)) {
+ if (!op->is_hook_valid(_hook)) {
delete op;
Dbg(pi_dbg_ctl, "in %s:%d: can't use this operator in hook=%s: %s(%s)",
filename, lineno, TSHttpHookNameLookup(_hook),
p.get_op().c_str(), p.get_arg().c_str());
diff --git a/plugins/header_rewrite/statement.cc
b/plugins/header_rewrite/statement.cc
index 8bce404a2d..351f5d572e 100644
--- a/plugins/header_rewrite/statement.cc
+++ b/plugins/header_rewrite/statement.cc
@@ -66,15 +66,9 @@ Statement::get_resource_ids() const
}
bool
-Statement::set_hook(TSHttpHookID hook)
+Statement::is_hook_valid(TSHttpHookID hook) const
{
- bool ret = std::find(_allowed_hooks.begin(), _allowed_hooks.end(), hook) !=
_allowed_hooks.end();
-
- if (ret) {
- _hook = hook;
- }
-
- return ret;
+ return std::find(_allowed_hooks.begin(), _allowed_hooks.end(), hook) !=
_allowed_hooks.end();
}
// This should be overridden for any Statement which only supports some hooks
diff --git a/plugins/header_rewrite/statement.h
b/plugins/header_rewrite/statement.h
index 4ed73ec681..0a354fc694 100644
--- a/plugins/header_rewrite/statement.h
+++ b/plugins/header_rewrite/statement.h
@@ -152,13 +152,8 @@ public:
Statement(const Statement &) = delete;
void operator=(const Statement &) = delete;
- // Which hook are we adding this statement to?
- bool set_hook(TSHttpHookID hook);
- TSHttpHookID
- get_hook() const
- {
- return _hook;
- }
+ // Validate that this statement is allowed on the given hook. Used during
parsing only.
+ bool is_hook_valid(TSHttpHookID hook) const;
// Which hooks are this "statement" applicable for? Used during parsing only.
void
@@ -300,7 +295,6 @@ protected:
private:
ResourceIDs _rsrc = RSRC_NONE;
- TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK;
std::vector<TSHttpHookID> _allowed_hooks;
std::string _config_filename;
int _config_lineno = 0;
diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc
index f7f80d7117..e00c9d13e8 100644
--- a/src/proxy/hdrs/HTTP.cc
+++ b/src/proxy/hdrs/HTTP.cc
@@ -693,7 +693,7 @@ http_hdr_url_set(HdrHeap *heap, HTTPHdrImpl *hh, URLImpl
*url)
void
http_hdr_status_set(HTTPHdrImpl *hh, HTTPStatus status)
{
- ink_assert(hh->m_polarity == HTTPType::RESPONSE);
+ ink_release_assert(hh->m_polarity == HTTPType::RESPONSE);
hh->u.resp.m_status = static_cast<int16_t>(status);
}
@@ -713,7 +713,7 @@ http_hdr_reason_get(HTTPHdrImpl *hh)
void
http_hdr_reason_set(HdrHeap *heap, HTTPHdrImpl *hh, std::string_view value,
bool must_copy)
{
- ink_assert(hh->m_polarity == HTTPType::RESPONSE);
+ ink_release_assert(hh->m_polarity == HTTPType::RESPONSE);
mime_str_u16_set(heap, value, &(hh->u.resp.m_ptr_reason),
&(hh->u.resp.m_len_reason), must_copy);
}
diff --git
a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bundle.replay.yaml
b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bundle.replay.yaml
index b444acaae8..af07d37972 100644
---
a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bundle.replay.yaml
+++
b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bundle.replay.yaml
@@ -176,6 +176,13 @@ autest:
args:
- "rules/rule_session_vars.conf"
+ - from: "http://www.example.com/from_18/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/to_18/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/set_status_in_if.conf"
+
metric_checks:
- metric: "proxy.process.plugin.header_rewrite.operators"
min: 1
@@ -1942,3 +1949,27 @@ sessions:
headers:
fields:
- [ X-Session-Seen, { value: "yes", as: equal } ]
+
+# Test 67: set-status inside if/endif at REMAP_PSEUDO_HOOK time.
+# Before the fix, the set-status operator inside an if/endif block retained
+# its default _hook (TS_HTTP_READ_RESPONSE_HDR_HOOK), so exec() called
+# TSHttpHdrStatusSet on a request buffer, corrupting the union and crashing.
+- transactions:
+ - client-request:
+ method: "GET"
+ version: "1.1"
+ url: /from_18/test
+ headers:
+ fields:
+ - [ Host, www.example.com ]
+ - [ uuid, 67 ]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [ Connection, close ]
+
+ proxy-response:
+ status: 403
diff --git
a/tests/gold_tests/pluginTest/header_rewrite/rules/set_status_in_if.conf
b/tests/gold_tests/pluginTest/header_rewrite/rules/set_status_in_if.conf
new file mode 100644
index 0000000000..f8b2b56cc0
--- /dev/null
+++ b/tests/gold_tests/pluginTest/header_rewrite/rules/set_status_in_if.conf
@@ -0,0 +1,27 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Minimal reproduction for set-status inside if/endif at remap time.
+# Before the fix, the operator's _hook defaulted to
+# TS_HTTP_READ_RESPONSE_HDR_HOOK, so exec() called TSHttpHdrStatusSet
+# on a request buffer, corrupting the header union and crashing.
+#
+cond %{REMAP_PSEUDO_HOOK}
+ if
+ cond %{CLIENT-URL:PATH} /from_18/
+ set-status 403
+ endif
