Fix nullptr crash in RecConfigOverrideFromEnvironment with runroot

include/records/RecCore.h

@@ -25,6 +25,8 @@
 
 #include <functional>
 #include <optional>
+#include <string>
+#include <utility>
 
 #include "tscore/Diags.h"
 
@@ -69,9 +71,32 @@ 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.
+};
+
+/// Human-readable 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";
+  default:
+    return "none";
+  }
+}
+
+// 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

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

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

src/records/RecordsConfigUtils.cc

@@ -49,17 +49,26 @@ 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.
     if (record->check != RECC_NULL && record->regex == nullptr) {
       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:

tests/gold_tests/records/records_runroot_precedence.test.py

@@ -0,0 +1,165 @@
+'''
+'''
+#  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')