Fix nullptr crash in RecConfigOverrideFromEnvironment with runroot

[brbzull0]

TL;DR

RecConfigOverrideFromEnvironment returned a plain string, so callers could only guess whether a value was overridden by comparing strings — and had no way to tell if the source was an env var or runroot, this was also generating UB because of a stirng created out of a nullptr.
This change makes the function return the resolved value paired with a typed RecConfigOverrideSource enum, giving callers explicit knowledge of what happened and why. Now it also returns either the env value or the runroot value for some runroot records.

A bit more details:

RecConfigOverrideFromEnvironment() previously returned a raw const char *
nullptr for the runroot case. All three callers fed that into
std::string, which is undefined behaviour and crashed on newer toolchains.

This PR:

1. Changes the return type to std::pair<std::string, RecConfigOverrideSource>
   with a new enum (NONE, ENV, RUNROOT) so callers know exactly who
   overrode the value.

2. For runroot-managed path records, returns the actual resolved Layout path
   (e.g. Layout::bindir, Layout::logdir) instead of an empty string. This
   means validation runs normally on every record — no special cases needed.

3. Replaces the RecConfigOverrideFromRunroot() strcmp chain with a constexpr
   lookup table that maps record names directly to Layout::* pointer-to-member.

4. Adds debug-level log messages at all three call sites:
   'record_name' overridden with 'value' by environment variable|runroot

5. Preserves nullptr for built-in defaults that are intentionally unset
   (e.g. proxy.config.ssl.keylog_file). This is safe because all
   nullptr-default records are RECD_STRING and RecDataSetFromString
   handles nullptr for that type.

[Copilot]

Pull request overview

Fixes a crash in ATS records.yaml parsing when runroot is enabled and certain PROXY_CONFIG_* path environment variables are unset, by ensuring RecConfigOverrideFromEnvironment() never returns nullptr for those records. This keeps the function’s contract intact and prevents exceptions when converting the returned C string into a std::string.

Changes:

• Update RecConfigOverrideFromEnvironment() to return the original record value (instead of nullptr) for runroot-managed path records when no env override exists.
• Add a gold test that unsets the four path env vars to reproduce the previous crash path and validates resulting record values via traffic_ctl.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/records/RecConfigParse.cc	Prevents nullptr from being returned during env override resolution under runroot, avoiding downstream std::string(nullptr) crashes.
tests/gold_tests/records/records_runroot_null_override.test.py	Adds regression coverage for the runroot + missing env-var scenario and verifies both env overrides and path-record behavior.

Comments suppressed due to low confidence (1)

src/records/RecConfigParse.cc:129

• RecConfigOverrideFromEnvironment() now returns value regardless of whether RecConfigOverrideFromRunroot(name) is true or false (when envval is unset), so this else if branch is redundant and adds an unnecessary RecConfigOverrideFromRunroot() call on the hot path while parsing every record. Consider removing the else if entirely (or otherwise making the runroot branch do something meaningfully different).

  if (envval) {
    return envval;
  } else if (RecConfigOverrideFromRunroot(name)) {
    return value;
  }

  return value;

[zwoop]

I think you should cleanup the else if, I think CoPilot said the same thing.

[brbzull0]

I think you should cleanup the else if, I think CoPilot said the same thing.

thanks for pointing this out. I think I may have introduced a side effect bug. I'll ammend and put this back for review.

[brbzull0]

[approve ci]

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[brbzull0]

I think you should cleanup the else if, I think CoPilot said the same thing.

Ok, there was a deeper issue at the end. My original change was actually causing troubles.
I changed the approach now and updated the code and PR description with the details.
Thanks.

Old

[zwoop]

Why is this empty multi-line string here? Typo / leftovers ?

[zwoop]

Can this happen? Should it be an assert ? Alternatively, instead of a default:, why not have NONE as the label, future proofing this?