From 9eb4f82e611aeb9a73580bab0494549048903ca9 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Fri, 24 Jul 2026 21:38:29 +0000 Subject: [PATCH] header_rewrite: reject bad run-plugin at config load A run-plugin whose target plugin failed to load left a null instance that tripped a release assert and aborted the server on the first request. Propagate the load failure as an exception so the rule is rejected at config load time (a reload simply keeps the running config), and guard exec() so a stray bad rule can never abort the process. --- plugins/header_rewrite/header_rewrite.cc | 31 ++-- plugins/header_rewrite/operators.cc | 36 ++-- plugins/header_rewrite/ruleset.cc | 25 ++- plugins/header_rewrite/ruleset.h | 2 +- .../header_rewrite_bad_run_plugin.test.py | 156 ++++++++++++++++++ 5 files changed, 201 insertions(+), 49 deletions(-) create mode 100644 tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc index b8c4f3c6dbf..80f720a935b 100644 --- a/plugins/header_rewrite/header_rewrite.cc +++ b/plugins/header_rewrite/header_rewrite.cc @@ -202,12 +202,12 @@ validate_rule_completion(RuleSet *rule, const std::string &fname, int lineno) bool RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, char *from_url, char *to_url) { - std::unique_ptr rule(nullptr); - std::string filename; - int lineno = 0; - ConditionGroup *group = nullptr; - std::stack group_stack; - std::stack if_stack; + std::unique_ptr rule(nullptr); + std::string filename; + int lineno = 0; + ConditionGroup *group = nullptr; + std::stack group_stack; + std::stack> if_stack; constexpr int MAX_IF_NESTING_DEPTH = 10; @@ -366,10 +366,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c throw std::runtime_error("maximum if nesting depth exceeded"); } - auto *op_if = new OperatorIf(); - - if_stack.push(op_if); - group = op_if->get_group(); // Set group to the new OperatorIf's group + if_stack.push(std::make_unique()); + group = if_stack.top()->get_group(); // Set group to the new OperatorIf's group Dbg(dbg_ctl, "Started nested OperatorIf, depth: %zu", if_stack.size()); } else if (p.is_endif()) { @@ -377,21 +375,20 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c throw std::runtime_error("endif without matching if"); } - OperatorIf *op_if = if_stack.top(); + auto op_if = std::move(if_stack.top()); if_stack.pop(); if (!if_stack.empty()) { auto *parent_sec = if_stack.top()->cur_section(); if (parent_sec->ops.oper) { - parent_sec->ops.oper->append(op_if); + parent_sec->ops.oper->append(op_if.release()); } else { - parent_sec->ops.oper.reset(op_if); + parent_sec->ops.oper = std::move(op_if); } group = if_stack.top()->get_group(); } else { - if (!rule->add_operator(op_if)) { - delete op_if; + if (!rule->add_operator(std::move(op_if))) { throw std::runtime_error("Failed to add nested OperatorIf to RuleSet"); } group = rule->get_group(); @@ -434,10 +431,6 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c // Check for unmatched if statements if (!if_stack.empty()) { TSError("[%s] %zu unmatched 'if' statement(s) without 'endif' in file: %s", PLUGIN_NAME, if_stack.size(), fname.c_str()); - while (!if_stack.empty()) { - delete if_stack.top(); - if_stack.pop(); - } return false; } diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 8013990285c..5d7bbab3860 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include "records/RecCore.h" #include "ts/ts.h" @@ -1272,8 +1273,7 @@ OperatorRunPlugin::initialize(Parser &p) auto plugin_args = p.get_value(); if (plugin_name.empty()) { - TSError("[%s] missing plugin name", PLUGIN_NAME); - return; + throw std::runtime_error("run-plugin missing plugin name"); } std::vector tokens; @@ -1284,15 +1284,10 @@ OperatorRunPlugin::initialize(Parser &p) tokens.push_back(token); } - // Create argc and argv - int argc = tokens.size() + 2; - char **argv = new char *[argc]; - - argv[0] = p.from_url(); - argv[1] = p.to_url(); + std::vector argv{p.from_url(), p.to_url()}; - for (size_t i = 0; i < tokens.size(); ++i) { - argv[i + 2] = const_cast(tokens[i].c_str()); + for (auto const &argument : tokens) { + argv.push_back(const_cast(argument.c_str())); } std::string error; @@ -1304,14 +1299,12 @@ OperatorRunPlugin::initialize(Parser &p) elevate_access = RecGetRecordInt("proxy.config.plugin.load_elevated").value_or(0); ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0); - _plugin = plugin_factory.getRemapPlugin(swoc::file::path(plugin_name), argc, const_cast(argv), error, + _plugin = plugin_factory.getRemapPlugin(swoc::file::path(plugin_name), static_cast(argv.size()), argv.data(), error, isPluginDynamicReloadEnabled()); } // done elevating access - delete[] argv; - if (!_plugin) { - TSError("[%s] Unable to load plugin '%s': %s", PLUGIN_NAME, plugin_name.c_str(), error.c_str()); + throw std::runtime_error("run-plugin unable to load plugin '" + std::string{plugin_name} + "': " + error); } } @@ -1326,7 +1319,11 @@ OperatorRunPlugin::initialize_hooks() bool OperatorRunPlugin::exec(const Resources &res) const { - TSReleaseAssert(_plugin != nullptr); + // Rejected at config load (see initialize); guard anyway so a stray bad rule can't abort the server. + if (!_plugin) { + Dbg(pi_dbg_ctl, "OperatorRunPlugin::exec skipped, plugin was not loaded"); + return true; + } if (res._rri && res.state.txnp) { _plugin->doRemap(res.state.txnp, res._rri); @@ -1654,7 +1651,7 @@ OperatorIf::new_section(Parser::CondClause clause) bool OperatorIf::add_operator(Parser &p, const char *filename, int lineno) { - Operator *op = operator_factory(p.get_op()); + std::unique_ptr op{operator_factory(p.get_op())}; if (!op) { TSError("[%s] Unknown operator: %s, file: %s, line: %d", PLUGIN_NAME, p.get_op().c_str(), filename, lineno); @@ -1667,7 +1664,6 @@ OperatorIf::add_operator(Parser &p, const char *filename, int lineno) try { op->initialize(p); } catch (std::exception const &ex) { - delete op; TSError("[%s] Failed to initialize operator: %s, file: %s, line: %d, error: %s", PLUGIN_NAME, p.get_op().c_str(), filename, lineno, ex.what()); return false; @@ -1675,10 +1671,10 @@ OperatorIf::add_operator(Parser &p, const char *filename, int lineno) // Add to current section if (_cur_section->ops.oper) { - _cur_section->ops.oper->append(op); + _cur_section->ops.oper->append(op.release()); } else { - _cur_section->ops.oper.reset(op); - _cur_section->ops.oper_mods = op->get_oper_modifiers(); + _cur_section->ops.oper = std::move(op); + _cur_section->ops.oper_mods = _cur_section->ops.oper->get_oper_modifiers(); } return true; diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc index acbeafc72f4..eae2ff0af15 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -19,6 +19,7 @@ // ruleset.cc: implementation of the ruleset class // // +#include #include #include "ruleset.h" @@ -89,14 +90,20 @@ RuleSet::make_condition(Parser &p, const char *filename, int lineno) bool RuleSet::add_operator(Parser &p, const char *filename, int lineno) { - Operator *op = operator_factory(p.get_op()); + std::unique_ptr op{operator_factory(p.get_op())}; - if (nullptr != op) { + if (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->set_config_location(filename, lineno); - op->initialize(p); + + try { + op->initialize(p); + } catch (std::exception const &ex) { + TSError("[%s] in %s:%d: failed to initialize operator %s: %s", PLUGIN_NAME, filename, lineno, p.get_op().c_str(), ex.what()); + return false; + } + 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()); TSError("[%s] in %s:%d: can't use this operator in hook=%s: %s(%s)", PLUGIN_NAME, filename, lineno, @@ -107,9 +114,9 @@ RuleSet::add_operator(Parser &p, const char *filename, int lineno) auto *cur_sec = _op_if.cur_section(); if (!cur_sec->ops.oper) { - cur_sec->ops.oper.reset(op); + cur_sec->ops.oper = std::move(op); } else { - cur_sec->ops.oper->append(op); + cur_sec->ops.oper->append(op.release()); } cur_sec->ops.oper_mods = static_cast(cur_sec->ops.oper_mods | cur_sec->ops.oper->get_oper_modifiers()); @@ -136,14 +143,14 @@ RuleSet::get_all_resource_ids() const } bool -RuleSet::add_operator(Operator *op) +RuleSet::add_operator(std::unique_ptr op) { auto *cur_sec = _op_if.cur_section(); if (!cur_sec->ops.oper) { - cur_sec->ops.oper.reset(op); + cur_sec->ops.oper = std::move(op); } else { - cur_sec->ops.oper->append(op); + cur_sec->ops.oper->append(op.release()); } // Update some ruleset state based on this new operator diff --git a/plugins/header_rewrite/ruleset.h b/plugins/header_rewrite/ruleset.h index 78680bc9d7e..0ad40f7183a 100644 --- a/plugins/header_rewrite/ruleset.h +++ b/plugins/header_rewrite/ruleset.h @@ -49,7 +49,7 @@ class RuleSet Condition *make_condition(Parser &p, const char *filename, int lineno); ResourceIDs get_all_resource_ids() const; bool add_operator(Parser &p, const char *filename, int lineno); - bool add_operator(Operator *op); + bool add_operator(std::unique_ptr op); ConditionGroup * get_group() diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py new file mode 100644 index 00000000000..c5371f5c9e9 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py @@ -0,0 +1,156 @@ +# 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. +''' +Verify header_rewrite rejects a run-plugin operator whose target plugin fails to +load. The failure must be caught at config load time, not aborted at request time. +''' + +Test.Summary = ''' +header_rewrite must reject a run-plugin whose target plugin fails to load, at +config load time, rather than aborting the server on the first request. +''' + +# Reproduce the reported crash: run-plugin against a plugin whose instance-init fails +# (conf_remap + a missing file) hands header_rewrite a null instance, which old code aborted on. +Test.SkipUnless( + Condition.PluginExists('header_rewrite.so'), + Condition.PluginExists('conf_remap.so'), +) + + +class TestBadRunPlugin: + '''Verify failed run-plugin initialization is rejected safely.''' + + ERROR_MARKER: str = 'run-plugin unable to load' + BAD_RULE_LINES: list[str] = [ + 'cond %{REMAP_PSEUDO_HOOK}', + ' run-plugin conf_remap.so no_such_conf_remap_file.yaml', + ] + NESTED_BAD_RULE_LINES: list[str] = [ + 'cond %{REMAP_PSEUDO_HOOK}', + ' if', + ' cond %{TRUE}', + ' run-plugin conf_remap.so no_such_conf_remap_file.yaml', + ' endif', + ] + + def __init__(self) -> None: + '''Configure startup and reload rejection scenarios.''' + self._configure_startup_rejection() + self._server = self._configure_origin_server() + self._ts = self._configure_traffic_server() + self._configure_baseline_request() + self._configure_bad_remap_install() + self._configure_failed_reload() + self._configure_post_reload_request() + self._ts.Disk.diags_log.Content = Testers.IncludesExpression( + self.ERROR_MARKER, 'the rejected reload should log the run-plugin failure') + + def _configure_startup_rejection(self) -> None: + '''Verify a bad top-level run-plugin fails startup cleanly.''' + ts = Test.MakeATSProcess("ts-startup", disable_log_checks=True) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', + }) + ts.Disk.MakeConfigFile('bad_run_plugin.conf').AddLines(self.BAD_RULE_LINES) + ts.Disk.remap_config.AddLine( + 'map http://startup.example.com/ http://127.0.0.1/ ' + '@plugin=header_rewrite.so @pparam=bad_run_plugin.conf') + + # Invalid remap.config triggers a controlled exit rather than SIGABRT. + ts.ReturnCode = 33 + ts.Ready = 0 + ts.Disk.diags_log.Content = Testers.IncludesExpression( + self.ERROR_MARKER, 'header_rewrite must report the failed run-plugin load') + ts.Disk.traffic_out.Content = Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'ATS must not initialize with a bad run-plugin config') + + tr = Test.AddTestRun("Bad run-plugin config fails startup instead of crashing") + tr.Processes.Default.Command = 'echo verifying startup rejection' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(ts) + + def _configure_origin_server(self) -> 'Process': + '''Configure the origin used to verify reload behavior.''' + server = Test.MakeOriginServer("server") + request_header = { + "headers": "GET / HTTP/1.1\r\nHost: reload.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + } + response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""} + server.addResponse("sessionfile.log", request_header, response_header) + return server + + def _configure_traffic_server(self) -> 'Process': + '''Configure ATS with a valid initial remap table.''' + ts = Test.MakeATSProcess("ts-reload", disable_log_checks=True) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', + }) + ts.Disk.MakeConfigFile('nested_bad_run_plugin.conf').AddLines(self.NESTED_BAD_RULE_LINES) + ts.Disk.remap_config.AddLine(f'map http://reload.example.com http://127.0.0.1:{self._server.Variables.Port}') + return ts + + def _configure_curl_run(self, name: str, expectation: str) -> 'TestRun': + '''Configure a request that verifies ATS still serves traffic.''' + tr = Test.AddTestRun(name) + tr.MakeCurlCommand( + f'--proxy 127.0.0.1:{self._ts.Variables.port} "http://reload.example.com" ' + '-H "Proxy-Connection: keep-alive" --verbose', + ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stderr = Testers.IncludesExpression('200 OK', expectation) + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + return tr + + def _configure_baseline_request(self) -> None: + '''Verify the valid initial configuration serves requests.''' + tr = self._configure_curl_run("Baseline request is served before reload", 'baseline request should be served') + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(self._ts) + + def _configure_bad_remap_install(self) -> None: + '''Replace remap.config with one containing a bad nested run-plugin.''' + tr = Test.AddTestRun("Install a remap.config with a bad run-plugin") + remap_path = self._ts.Disk.remap_config.AbsPath + tr.Disk.File(remap_path, id="remap_bad", typename="ats:config") + tr.Disk.remap_bad.AddLine( + f'map http://reload.example.com http://127.0.0.1:{self._server.Variables.Port} ' + '@plugin=header_rewrite.so @pparam=nested_bad_run_plugin.conf') + tr.Processes.Default.Command = 'echo installed bad remap.config' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + + def _configure_failed_reload(self) -> None: + '''Verify the bad remap table is rejected without stopping ATS.''' + tr = Test.AddConfigReload( + self._ts, expect="fail", delay_start=2, description="Reload with bad run-plugin must be rejected, not fatal") + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + + def _configure_post_reload_request(self) -> None: + '''Verify the rejected reload leaves the old configuration active.''' + self._configure_curl_run( + "Server still serves the old config after the rejected reload", 'old config should still serve after a rejected reload') + + +TestBadRunPlugin()