This is an automated email from the ASF dual-hosted git repository.
dmeden 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 a39c1ae39f Fix nullptr crash in RecConfigOverrideFromEnvironment with
runroot (#12917)
a39c1ae39f is described below
commit a39c1ae39f775a863c9a74ebc530884c7122258b
Author: Damian Meden <[email protected]>
AuthorDate: Tue Mar 10 11:43:30 2026 +0100
Fix nullptr crash in RecConfigOverrideFromEnvironment with runroot (#12917)
* RecConfigOverrideFromEnvironment: return resolved value and override
source
Change the return type to std::pair<std::string, RecConfigOverrideSource>
so callers know whether a record was overridden and by whom (ENV or
RUNROOT).
When runroot is active, return the actual Layout path (e.g. Layout::bindir)
instead of the old nullptr which caused undefined behaviour when converted
to std::string. Replace the RecConfigOverrideFromRunroot() strcmp chain
with a constexpr lookup table mapping record names to Layout members.
All three callers now log the override source at debug level. Preserve
nullptr for built-in defaults that are intentionally unset.
Adds records_runroot_precedence autest.
---
include/records/RecCore.h | 33 ++++-
src/records/RecConfigParse.cc | 54 ++++---
src/records/RecYAMLDecoder.cc | 9 +-
src/records/RecordsConfigUtils.cc | 17 ++-
.../records/records_runroot_precedence.test.py | 163 +++++++++++++++++++++
5 files changed, 242 insertions(+), 34 deletions(-)
diff --git a/include/records/RecCore.h b/include/records/RecCore.h
index d0a6ed5b4f..a355bd39cc 100644
--- a/include/records/RecCore.h
+++ b/include/records/RecCore.h
@@ -25,6 +25,8 @@
#include <functional>
#include <optional>
+#include <string>
+#include <utility>
#include "tscore/Diags.h"
@@ -69,9 +71,34 @@ std::string RecConfigReadConfigPath(const char
*file_variable, const char *defau
// Return a copy of the persistent stats file. This is
$RUNTIMEDIR/records.snap.
std::string RecConfigReadPersistentStatsPath();
-// Test whether the named configuration value is overridden by an environment
variable. Return either
-// the overridden value, or the original value. Caller MUST NOT free the
result.
-const char *RecConfigOverrideFromEnvironment(const char *name, const char
*value);
+/// Indicates why RecConfigOverrideFromEnvironment() chose its returned value.
+enum class RecConfigOverrideSource {
+ NONE, ///< No override — the original value was kept.
+ ENV, ///< Overridden by a PROXY_CONFIG_* environment variable.
+ RUNROOT, ///< Overridden with the resolved Layout path because runroot
manages this record.
+};
+
+/// Label for the override source (for logging).
+constexpr const char *
+RecConfigOverrideSourceName(RecConfigOverrideSource src)
+{
+ switch (src) {
+ case RecConfigOverrideSource::ENV:
+ return "environment variable";
+ case RecConfigOverrideSource::RUNROOT:
+ return "runroot";
+ case RecConfigOverrideSource::NONE:
+ return "none";
+ default:
+ return "unknown";
+ }
+}
+
+// Test whether the named configuration value is overridden by the execution
+// environment — either a PROXY_CONFIG_* environment variable or the runroot
+// mechanism. Returns the resolved value together with the source that
+// produced it.
+std::pair<std::string, RecConfigOverrideSource>
RecConfigOverrideFromEnvironment(const char *name, const char *value);
//-------------------------------------------------------------------------
// Stat Registration
diff --git a/src/records/RecConfigParse.cc b/src/records/RecConfigParse.cc
index 0819137f7d..d80a1237af 100644
--- a/src/records/RecConfigParse.cc
+++ b/src/records/RecConfigParse.cc
@@ -84,24 +84,20 @@ RecFileImport_Xmalloc(const char *file, char **file_buf,
int *file_size)
}
//-------------------------------------------------------------------------
-// RecConfigOverrideFromRunroot
+// Records whose paths are managed by runroot. When runroot is active the
+// value from records.yaml is replaced with the resolved Layout path.
//-------------------------------------------------------------------------
-bool
-RecConfigOverrideFromRunroot(const char *name)
-{
- if (!get_runroot().empty()) {
- if (!strcmp(name, "proxy.config.bin_path") || !strcmp(name,
"proxy.config.local_state_dir") ||
- !strcmp(name, "proxy.config.log.logfile_dir") || !strcmp(name,
"proxy.config.plugin.plugin_dir")) {
- return true;
- }
- }
- return false;
-}
+static constexpr std::pair<std::string_view, std::string Layout::*>
runroot_records[] = {
+ {"proxy.config.bin_path", &Layout::bindir },
+ {"proxy.config.local_state_dir", &Layout::runtimedir},
+ {"proxy.config.log.logfile_dir", &Layout::logdir },
+ {"proxy.config.plugin.plugin_dir", &Layout::libexecdir},
+};
//-------------------------------------------------------------------------
// RecConfigOverrideFromEnvironment
//-------------------------------------------------------------------------
-const char *
+std::pair<std::string, RecConfigOverrideSource>
RecConfigOverrideFromEnvironment(const char *name, const char *value)
{
ats_scoped_str envname(ats_strdup(name));
@@ -121,12 +117,18 @@ RecConfigOverrideFromEnvironment(const char *name, const
char *value)
envval = getenv(envname.get());
if (envval) {
- return envval;
- } else if (RecConfigOverrideFromRunroot(name)) {
- return nullptr;
+ return {envval, RecConfigOverrideSource::ENV};
+ }
+
+ if (!get_runroot().empty()) {
+ for (auto const &[rec_name, member] : runroot_records) {
+ if (rec_name == name) {
+ return {Layout::get()->*member, RecConfigOverrideSource::RUNROOT};
+ }
+ }
}
- return value;
+ return {value ? value : "", RecConfigOverrideSource::NONE};
}
//-------------------------------------------------------------------------
@@ -141,10 +143,9 @@ RecConfigFileParse(const char *path,
RecConfigEntryCallback handler)
const char *line;
int line_num;
- char *rec_type_str, *name_str, *data_type_str, *data_str;
- const char *value_str;
- RecT rec_type;
- RecDataT data_type;
+ char *rec_type_str, *name_str, *data_type_str, *data_str;
+ RecT rec_type;
+ RecDataT data_type;
Tokenizer line_tok("\r\n");
tok_iter_state line_tok_state;
@@ -245,8 +246,15 @@ RecConfigFileParse(const char *path,
RecConfigEntryCallback handler)
}
// OK, we parsed the record, send it to the handler ...
- value_str = RecConfigOverrideFromEnvironment(name_str, data_str);
- handler(rec_type, data_type, name_str, value_str, value_str == data_str ?
REC_SOURCE_EXPLICIT : REC_SOURCE_ENV);
+ {
+ auto [value_str, override_source] =
RecConfigOverrideFromEnvironment(name_str, data_str);
+ if (override_source != RecConfigOverrideSource::NONE) {
+ RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", name_str,
value_str.c_str(),
+ RecConfigOverrideSourceName(override_source));
+ }
+ handler(rec_type, data_type, name_str, value_str.c_str(),
+ override_source == RecConfigOverrideSource::NONE ?
REC_SOURCE_EXPLICIT : REC_SOURCE_ENV);
+ }
// update our g_rec_config_contents_xxx
g_rec_config_contents_ht.emplace(name_str);
diff --git a/src/records/RecYAMLDecoder.cc b/src/records/RecYAMLDecoder.cc
index 29f2f12fa9..1bb7b9e45e 100644
--- a/src/records/RecYAMLDecoder.cc
+++ b/src/records/RecYAMLDecoder.cc
@@ -156,11 +156,12 @@ SetRecordFromYAMLNode(CfgNode const &field, swoc::Errata
&errata)
std::string field_value = field.value_node.as<std::string>(); // in case of
a string, the library will give us the literal
// 'null'
which is exactly what we want.
- std::string value_str =
RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str());
- RecSourceT source = (field_value == value_str ? REC_SOURCE_EXPLICIT :
REC_SOURCE_ENV);
+ auto [value_str, override_source] =
RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str());
+ RecSourceT source = (override_source ==
RecConfigOverrideSource::NONE) ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV;
- if (source == REC_SOURCE_ENV) {
- errata.note(ERRATA_DEBUG, "'{}' was override with '{}' using an env
variable", record_name, value_str);
+ if (override_source != RecConfigOverrideSource::NONE) {
+ errata.note(ERRATA_DEBUG, "'{}' overridden with '{}' by {}", record_name,
value_str,
+ RecConfigOverrideSourceName(override_source));
}
if (!check_expr.empty() && RecordValidityCheck(value_str.c_str(),
check_type, check_expr.c_str()) == false) {
diff --git a/src/records/RecordsConfigUtils.cc
b/src/records/RecordsConfigUtils.cc
index f572a95749..9d24451061 100644
--- a/src/records/RecordsConfigUtils.cc
+++ b/src/records/RecordsConfigUtils.cc
@@ -49,9 +49,14 @@ initialize_record(const RecordElement *record, void *)
access = record->access;
if (REC_TYPE_IS_CONFIG(type)) {
- const char *value = RecConfigOverrideFromEnvironment(record->name,
record->value);
- RecData data = {0};
- RecSourceT source = value == record->value ? REC_SOURCE_DEFAULT :
REC_SOURCE_ENV;
+ auto [value, override_source] =
RecConfigOverrideFromEnvironment(record->name, record->value);
+ RecData data = {0};
+ RecSourceT source = (override_source ==
RecConfigOverrideSource::NONE) ? REC_SOURCE_DEFAULT : REC_SOURCE_ENV;
+
+ if (override_source != RecConfigOverrideSource::NONE) {
+ RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", record->name,
value.c_str(),
+ RecConfigOverrideSourceName(override_source));
+ }
// If you specify a consistency check, you have to specify a regex
expression. We abort here
// so that this breaks QA completely.
@@ -59,7 +64,11 @@ initialize_record(const RecordElement *record, void *)
ink_fatal("%s has a consistency check but no regular expression",
record->name);
}
- RecDataSetFromString(record->value_type, &data, value);
+ // When the built-in default is nullptr and no override was applied,
preserve
+ // nullptr so optional records (e.g. keylog_file, groups_list) stay unset.
+ const char *value_ptr =
+ (override_source == RecConfigOverrideSource::NONE && record->value ==
nullptr) ? nullptr : value.c_str();
+ RecDataSetFromString(record->value_type, &data, value_ptr);
RecErrT reg_status{REC_ERR_FAIL};
switch (record->value_type) {
case RECD_INT:
diff --git a/tests/gold_tests/records/records_runroot_precedence.test.py
b/tests/gold_tests/records/records_runroot_precedence.test.py
new file mode 100644
index 0000000000..49077c174c
--- /dev/null
+++ b/tests/gold_tests/records/records_runroot_precedence.test.py
@@ -0,0 +1,163 @@
+# 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.
+
+import os
+
+Test.Summary = '''
+Verify that when runroot is active, path records from records.yaml are
+overridden by the resolved Layout paths (precedence: env var > runroot >
records.yaml).
+
+When --run-root (or TS_RUNROOT) is set and the PROXY_CONFIG_* environment
+variables for path records are unset, RecConfigOverrideFromEnvironment()
+returns the actual Layout path (e.g. Layout::bindir, Layout::logdir) which
+was populated from runroot.yaml — effectively making runroot.yaml override
+records.yaml for these path records.
+'''
+Test.ContinueOnFail = True
+Test.SkipUnless(
+ Test.Variables.BINDIR.startswith(Test.Variables.PREFIX), "need to
guarantee bin path starts with prefix for runroot")
+
+ts = Test.MakeATSProcess("ts")
+ts_dir = os.path.join(Test.RunDirectory, "ts")
+
+# Set deliberately WRONG values in records.yaml for all 4 runroot-managed
+# path records. If runroot override works, these values must NOT be used.
+ts.Disk.records_config.append_to_document(
+ '''
+ bin_path: wrong_bin_path
+ local_state_dir: wrong_runtime
+ log:
+ logfile_dir: wrong_log
+ plugin:
+ plugin_dir: wrong_plugin
+''')
+
+# Test 3 setup: env var that must win over both runroot and records.yaml.
+ts.Env['PROXY_CONFIG_DIAGS_DEBUG_TAGS'] = 'env_wins'
+ts.Disk.records_config.update('''
+ diags:
+ debug:
+ enabled: 0
+ tags: config_value
+ ''')
+
+# Build the ATS command:
+# - Unset the 4 path env vars (the test framework always sets them,
+# which masks the runroot code path).
+# - Set TS_RUNROOT to the sandbox dir so the runroot mechanism activates.
+original_cmd = ts.Command
+ts.Command = (
+ "env"
+ " -u PROXY_CONFIG_BIN_PATH"
+ " -u PROXY_CONFIG_LOCAL_STATE_DIR"
+ " -u PROXY_CONFIG_LOG_LOGFILE_DIR"
+ " -u PROXY_CONFIG_PLUGIN_PLUGIN_DIR"
+ f" TS_RUNROOT={ts_dir}"
+ f" {original_cmd}")
+
+# ---------------------------------------------------------------------------
+# Test 0: Create runroot.yaml that maps to the sandbox layout, then start ATS.
+#
+# The runroot.yaml must exist before ATS starts because TS_RUNROOT triggers
+# Layout::runroot_setup() during initialization. We write a runroot.yaml
+# whose paths match the sandbox structure the test framework already created
+# (traffic_layout init would create a different FHS-style layout that does
+# not match the sandbox, so we write it manually).
+# ---------------------------------------------------------------------------
+runroot_yaml = os.path.join(ts_dir, 'runroot.yaml')
+
+runroot_lines = [
+ f"prefix: {ts_dir}",
+ f"bindir: {os.path.join(ts_dir, 'bin')}",
+ f"sbindir: {os.path.join(ts_dir, 'bin')}",
+ f"sysconfdir: {os.path.join(ts_dir, 'config')}",
+ f"logdir: {os.path.join(ts_dir, 'log')}",
+ f"libexecdir: {os.path.join(ts_dir, 'plugin')}",
+ f"localstatedir: {os.path.join(ts_dir, 'runtime')}",
+ f"runtimedir: {os.path.join(ts_dir, 'runtime')}",
+ f"cachedir: {os.path.join(ts_dir, 'cache')}",
+]
+runroot_content = "\\n".join(runroot_lines) + "\\n"
+
+tr = Test.AddTestRun("Create runroot.yaml")
+tr.Processes.Default.Command = f"mkdir -p {ts_dir} && printf
'{runroot_content}' > {runroot_yaml}"
+tr.Processes.Default.ReturnCode = 0
+
+# ---------------------------------------------------------------------------
+# Test 1: Start ATS with runroot active
+# ---------------------------------------------------------------------------
+tr = Test.AddTestRun("Start ATS with runroot")
+tr.Processes.Default.Command = 'echo start'
+tr.Processes.Default.ReturnCode = 0
+tr.Processes.Default.StartBefore(ts)
+tr.StillRunningAfter = ts
+
+# ATS must not crash (the original nullptr bug) and must complete startup.
+ts.Disk.traffic_out.Content = Testers.ExcludesExpression(
+ "basic_string", "must not crash with 'basic_string: construction from
null'")
+ts.Disk.traffic_out.Content += Testers.ContainsExpression("records parsing
completed", "ATS should complete records parsing")
+
+# Verify the override log messages appear in traffic.out.
+# The errata notes from RecYAMLDecoder are printed by RecCoreInit().
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "'proxy.config.bin_path' overridden with .* by runroot", "bin_path
override by runroot must be logged")
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "'proxy.config.local_state_dir' overridden with .* by runroot",
"local_state_dir override by runroot must be logged")
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "'proxy.config.log.logfile_dir' overridden with .* by runroot",
"logfile_dir override by runroot must be logged")
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "'proxy.config.plugin.plugin_dir' overridden with .* by runroot",
"plugin_dir override by runroot must be logged")
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "'proxy.config.diags.debug.tags' overridden with 'env_wins' by environment
variable",
+ "diags.debug.tags override by environment variable must be logged")
+
+# ---------------------------------------------------------------------------
+# Test 2: Verify path records do NOT contain the records.yaml values.
+#
+# Because runroot is active and env vars are unset, the records should hold
+# the resolved Layout paths from runroot.yaml, not the records.yaml values.
+# ---------------------------------------------------------------------------
+tr = Test.AddTestRun("Verify runroot overrides records.yaml for path records")
+tr.Processes.Default.Command = (
+ 'traffic_ctl config get'
+ ' proxy.config.bin_path'
+ ' proxy.config.local_state_dir'
+ ' proxy.config.log.logfile_dir'
+ ' proxy.config.plugin.plugin_dir')
+tr.Processes.Default.Env = ts.Env
+tr.Processes.Default.ReturnCode = 0
+tr.StillRunningAfter = ts
+
+# The deliberately wrong records.yaml values must NOT appear.
+tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
+ 'wrong_bin_path', 'bin_path must be overridden by runroot, not
records.yaml')
+tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
+ 'wrong_runtime', 'local_state_dir must be overridden by runroot, not
records.yaml')
+tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
+ 'wrong_log', 'logfile_dir must be overridden by runroot, not records.yaml')
+tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
+ 'wrong_plugin', 'plugin_dir must be overridden by runroot, not
records.yaml')
+
+# ---------------------------------------------------------------------------
+# Test 3: Verify env vars still take highest precedence over runroot.
+# ---------------------------------------------------------------------------
+tr = Test.AddTestRun("Verify env var overrides both runroot and records.yaml")
+tr.Processes.Default.Command = 'traffic_ctl config get
proxy.config.diags.debug.tags'
+tr.Processes.Default.Env = ts.Env
+tr.Processes.Default.ReturnCode = 0
+tr.StillRunningAfter = ts
+tr.Processes.Default.Streams.stdout = Testers.ContainsExpression(
+ 'proxy.config.diags.debug.tags: env_wins', 'Env var must override both
runroot and records.yaml')
