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()