From 079936f921b58b8b2b216b42bf403ad0b41cd9c5 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Fri, 24 Jul 2026 15:38:29 -0600 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/operators.cc | 12 +- plugins/header_rewrite/ruleset.cc | 11 +- .../header_rewrite_bad_run_plugin.test.py | 127 ++++++++++++++++++ 3 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 133991228cd..737098285e0 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" @@ -1282,8 +1283,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; @@ -1321,7 +1321,7 @@ OperatorRunPlugin::initialize(Parser &p) 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); } } @@ -1336,7 +1336,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); diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc index acbeafc72f4..d52de21236d 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -20,6 +20,7 @@ // // #include +#include #include "ruleset.h" #include "factory.h" @@ -94,7 +95,15 @@ 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->set_config_location(filename, lineno); - op->initialize(p); + + try { + op->initialize(p); + } catch (std::exception const &ex) { + delete op; + 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), 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..8d7a7ed2b89 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py @@ -0,0 +1,127 @@ +''' +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. +''' +# 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. + +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'), +) + +# The unique substring header_rewrite logs when a run-plugin target won't load. +error_marker = 'run-plugin unable to load' +bad_rule_lines = [ + 'cond %{REMAP_PSEUDO_HOOK}', + ' run-plugin conf_remap.so no_such_conf_remap_file.yaml', +] + +# ---------------------------------------------------------------------------- +# Part 1: A bad run-plugin config must fail startup cleanly (no abort/core). +# ---------------------------------------------------------------------------- +# disable_log_checks: we deliberately expect ERROR: lines in diags.log. +ts_start = Test.MakeATSProcess("ts-startup", disable_log_checks=True) +ts_start.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', +}) + +ts_start.Disk.MakeConfigFile('bad_run_plugin.conf').AddLines(bad_rule_lines) +ts_start.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 -> Emergency() -> _exit(33): a controlled shutdown, not a SIGABRT crash. +ts_start.ReturnCode = 33 +ts_start.Ready = 0 # It never reaches "fully initialized"; don't wait for readiness. +ts_start.Disk.diags_log.Content = Testers.IncludesExpression(error_marker, 'header_rewrite must report the failed run-plugin load') +ts_start.Disk.traffic_out.Content = Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'ATS must not initialize with a bad run-plugin config') + +tr_start = Test.AddTestRun("Bad run-plugin config fails startup instead of crashing") +tr_start.Processes.Default.Command = 'echo verifying startup rejection' +tr_start.Processes.Default.ReturnCode = 0 +tr_start.Processes.Default.StartBefore(ts_start) + +# ---------------------------------------------------------------------------- +# Part 2: A bad run-plugin config on reload must be rejected, leaving the running +# server untouched (the reason throwing here is safe, not fatal). +# ---------------------------------------------------------------------------- +ts = Test.MakeATSProcess("ts-reload", disable_log_checks=True) +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) + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', +}) + +# The bad rule file exists from the start; it is only referenced after the reload. +ts.Disk.MakeConfigFile('bad_run_plugin.conf').AddLines(bad_rule_lines) + +# Initial config is valid (plain map, no plugin) so ATS comes up and serves. +ts.Disk.remap_config.AddLine(f'map http://reload.example.com http://127.0.0.1:{server.Variables.Port}') + + +def curl(tr, description): + tr.MakeCurlCommand( + f'--proxy 127.0.0.1:{ts.Variables.port} "http://reload.example.com" -H "Proxy-Connection: keep-alive" --verbose', ts=ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stderr = Testers.IncludesExpression('200 OK', description) + tr.StillRunningAfter = ts + tr.StillRunningAfter = server + + +tr1 = Test.AddTestRun("Baseline request is served before reload") +tr1.Processes.Default.StartBefore(server) +tr1.Processes.Default.StartBefore(ts) +curl(tr1, 'baseline request should be served') + +# Overwrite remap.config with a mapping whose header_rewrite rule has a bad run-plugin. +tr2 = Test.AddTestRun("Install a remap.config with a bad run-plugin") +remap_path = ts.Disk.remap_config.AbsPath +tr2.Disk.File(remap_path, id="remap_bad", typename="ats:config") +tr2.Disk.remap_bad.AddLine( + f'map http://reload.example.com http://127.0.0.1:{server.Variables.Port} ' + '@plugin=header_rewrite.so @pparam=bad_run_plugin.conf') +tr2.Processes.Default.Command = 'echo installed bad remap.config' +tr2.Processes.Default.ReturnCode = 0 +tr2.Processes.Default.Env = ts.Env +tr2.StillRunningAfter = ts +tr2.StillRunningAfter = server + +# The reload must fail (traffic_ctl exits 2) and ATS must keep running. +tr_reload = Test.AddConfigReload( + ts, expect="fail", delay_start=2, description="Reload with bad run-plugin must be rejected, not fatal") +tr_reload.StillRunningAfter = ts +tr_reload.StillRunningAfter = server + +# The rejected reload keeps the old table, so requests are still served. +tr3 = Test.AddTestRun("Server still serves the old config after the rejected reload") +curl(tr3, 'old config should still serve after a rejected reload') + +ts.Disk.diags_log.Content = Testers.IncludesExpression(error_marker, 'the rejected reload should log the run-plugin failure')