From d1764f7aa22dce2be615c3287cbdfe890758e80e Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Thu, 26 Mar 2026 11:35:59 -0500 Subject: [PATCH] Fix set-status crash inside if/endif at remap time Operators inside an if/endif block never had set_hook() called on them, so _hook retained its default value of TS_HTTP_READ_RESPONSE_HDR_HOOK. When set-status executed at REMAP_PSEUDO_HOOK time, exec() took the response-hook branch and called TSHttpHdrStatusSet on a request buffer, corrupting the header union and crashing. Fix the operator to check TSHttpHdrTypeGet() at runtime instead of trusting get_hook(). Removed _hook and get_hook() from operators. set_hook() becomes is_hook_valid() in Statement and RuleSet to only validate hook compatibility without mutating _hook. Upgrade the HTTPHdr::status_set/reason_set asserts to release_assert so that writing response fields into a request buffer is caught immediately rather than silently corrupting memory. (cherry picked from commit e4728cc49625cc3767f77b1ecd8ad0ebdf17d016) --- 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 ++-- 6 files changed, 16 insertions(+), 34 deletions(-) diff --git a/include/proxy/hdrs/HTTP.h b/include/proxy/hdrs/HTTP.h index 22d6c45a7b3..22195e63669 100644 --- a/include/proxy/hdrs/HTTP.h +++ b/include/proxy/hdrs/HTTP.h @@ -1091,7 +1091,7 @@ inline void HTTPHdr::status_set(HTTPStatus status) { ink_assert(valid()); - ink_assert(m_http->m_polarity == HTTP_TYPE_RESPONSE); + ink_release_assert(m_http->m_polarity == HTTP_TYPE_RESPONSE); http_hdr_status_set(m_http, status); } @@ -1115,7 +1115,7 @@ inline void HTTPHdr::reason_set(const char *value, int length) { ink_assert(valid()); - ink_assert(m_http->m_polarity == HTTP_TYPE_RESPONSE); + ink_release_assert(m_http->m_polarity == HTTP_TYPE_RESPONSE); http_hdr_reason_set(m_heap, m_http, value, length, true); } diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 0b6b30375b1..f572ae7f5ca 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -202,19 +202,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())); - 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())); + 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())); - break; } Dbg(pi_dbg_ctl, "OperatorSetStatus::exec() invoked with status=%d", _status.get_int_value()); @@ -602,7 +596,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); diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc index 501315e0634..c301522382d 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -53,7 +53,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()); @@ -82,7 +82,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 ca840d7a9b5..f4335e243ef 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 510957837ab..724017beb57 100644 --- a/plugins/header_rewrite/statement.h +++ b/plugins/header_rewrite/statement.h @@ -143,13 +143,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 @@ -244,7 +239,6 @@ class Statement private: ResourceIDs _rsrc = RSRC_NONE; - TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK; std::vector _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 a4d350ee9ef..e3fc63d7638 100644 --- a/src/proxy/hdrs/HTTP.cc +++ b/src/proxy/hdrs/HTTP.cc @@ -754,7 +754,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 == HTTP_TYPE_RESPONSE); + ink_release_assert(hh->m_polarity == HTTP_TYPE_RESPONSE); hh->u.resp.m_status = status; } @@ -775,7 +775,7 @@ http_hdr_reason_get(HTTPHdrImpl *hh, int *length) void http_hdr_reason_set(HdrHeap *heap, HTTPHdrImpl *hh, const char *value, int length, bool must_copy) { - ink_assert(hh->m_polarity == HTTP_TYPE_RESPONSE); + ink_release_assert(hh->m_polarity == HTTP_TYPE_RESPONSE); mime_str_u16_set(heap, value, length, &(hh->u.resp.m_ptr_reason), &(hh->u.resp.m_len_reason), must_copy); }