diff --git a/include/proxy/hdrs/HTTP.h b/include/proxy/hdrs/HTTP.h index dc2d7b61f7f..8efffcc97c3 100644 --- a/include/proxy/hdrs/HTTP.h +++ b/include/proxy/hdrs/HTTP.h @@ -1088,7 +1088,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); } @@ -1112,7 +1112,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 20207bd24b8..b64b4ec469b 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -198,19 +198,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(_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(_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(_status.get_int_value()), PLUGIN_NAME); - break; } Dbg(pi_dbg_ctl, "OperatorSetStatus::exec() invoked with status=%d", _status.get_int_value()); @@ -601,7 +595,7 @@ OperatorSetRedirect::exec(const Resources &res) const const_cast(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(_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 28fda37a511..ba17407f203 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -66,7 +66,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->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()); @@ -93,7 +93,7 @@ RuleSet::add_operator(Parser &p, const char *filename, int lineno) if (nullptr != op) { 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->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 6e2607da634..210e7c050f1 100644 --- a/plugins/header_rewrite/statement.cc +++ b/plugins/header_rewrite/statement.cc @@ -48,15 +48,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 2557d4482c1..dcc3e2fb89e 100644 --- a/plugins/header_rewrite/statement.h +++ b/plugins/header_rewrite/statement.h @@ -144,13 +144,8 @@ class Statement 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 @@ -267,7 +262,6 @@ class Statement private: ResourceIDs _rsrc = RSRC_NONE; - TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK; std::vector _allowed_hooks; bool _initialized = false; }; diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index f0db0665e7f..ee082a74313 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(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 b3c057d07a5..8efa5072363 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" + log_validation: traffic_out: excludes: @@ -1880,3 +1887,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 00000000000..f8b2b56cc0f --- /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