This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 14bcf29953 header_rewrite: reject bad run-plugin at config load
(#13493)
14bcf29953 is described below
commit 14bcf29953a07797e4661eac0a812bfa907b5b7d
Author: Brian Neradt <[email protected]>
AuthorDate: Wed Aug 5 16:59:51 2026 -0500
header_rewrite: reject bad run-plugin at config load (#13493)
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.
Co-authored-by: Leif Hedstrom <[email protected]>
---
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(-)
diff --git a/plugins/header_rewrite/header_rewrite.cc
b/plugins/header_rewrite/header_rewrite.cc
index b8c4f3c6db..80f720a935 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<RuleSet> rule(nullptr);
- std::string filename;
- int lineno = 0;
- ConditionGroup *group = nullptr;
- std::stack<ConditionGroup *> group_stack;
- std::stack<OperatorIf *> if_stack;
+ std::unique_ptr<RuleSet> rule(nullptr);
+ std::string filename;
+ int lineno = 0;
+ ConditionGroup *group = nullptr;
+ std::stack<ConditionGroup *> group_stack;
+ std::stack<std::unique_ptr<OperatorIf>> 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<OperatorIf>());
+ 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 8013990285..5d7bbab386 100644
--- a/plugins/header_rewrite/operators.cc
+++ b/plugins/header_rewrite/operators.cc
@@ -23,6 +23,7 @@
#include <cstring>
#include <algorithm>
#include <iomanip>
+#include <stdexcept>
#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<std::string> 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<char *> argv{p.from_url(), p.to_url()};
- for (size_t i = 0; i < tokens.size(); ++i) {
- argv[i + 2] = const_cast<char *>(tokens[i].c_str());
+ for (auto const &argument : tokens) {
+ argv.push_back(const_cast<char *>(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<char **>(argv), error,
+ _plugin = plugin_factory.getRemapPlugin(swoc::file::path(plugin_name),
static_cast<int>(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<Operator> 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 acbeafc72f..eae2ff0af1 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 <exception>
#include <string>
#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<Operator> 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<OperModifiers>(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<Operator> 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 78680bc9d7..0ad40f7183 100644
--- a/plugins/header_rewrite/ruleset.h
+++ b/plugins/header_rewrite/ruleset.h
@@ -49,7 +49,7 @@ public:
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<Operator> 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 0000000000..c5371f5c9e
--- /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()
