From 8407595104997d0d58035f16caa85b4b22c7d99a Mon Sep 17 00:00:00 2001 From: Vincent Hengel Date: Wed, 16 Sep 2026 20:34:39 +0200 Subject: [PATCH] fix(base): an Option owns the string it is given SetFromString stored the caller's pointer, so a string option was only as alive as whatever buffer it was parsed out of. InitOptionsFromEnv reads each variable into a local StringU8 and hands over its c_str(), which dies at the end of the loop iteration: every const char* option set from the environment has pointed at freed memory since base stopped calling getenv (#14). It does not read as a crash. rx takes its capture path from RX_UI_SHOT=, and with the dangling value the engine wrote a 51 KB png to a filename made of whatever the freed bytes happened to spell. Whoever calls SetFromString next gets a different kind of wrong. The other two callers show the shape of the trap. option_file.cc knew, and interns every value into a pool that outlives the options (InternValue). recreation's platform-profile loader did not, and passes the c_str() of a std::string that goes out of scope one line later. Three call sites, two of them holding it wrong, is an API telling you where the ownership belongs: with the option. ParseFromString now copies into a buffer the option keeps, so a value survives its source, Reset() drops it again, and callers can pass anything they have at hand. Only const char* options carry the buffer -- detail::OwnedText is empty for every other T -- so an Option is the size it always was. base/option_test.cc covers what nothing covered: parsing per type, a value outliving the buffer it came from, the same through the environment, Reset, and a rejected parse leaving the old value in place. Three of the six fail without this change. --- base/option.h | 43 +++++++++++++++++++---- base/option_test.cc | 86 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 base/option_test.cc diff --git a/base/option.h b/base/option.h index 02c473de..0c49626d 100644 --- a/base/option.h +++ b/base/option.h @@ -15,13 +15,15 @@ #include #include #include +#include namespace base { namespace detail { // Parse `text` into `out`. Returns false (leaving `out` untouched) when the -// text is not a valid value for T. Covers bool, the integral and floating -// types, and const char* (which is bound to the source string, not copied). +// text is not a valid value for T. Covers bool and the integral and floating +// types; a const char* option does not come through here, because it has to +// own what it is given (Option::ParseFromString). template inline bool ParseOption(const char* text, T& out) { if constexpr (base::is_same_v) { @@ -44,9 +46,6 @@ inline bool ParseOption(const char* text, T& out) { return true; } return false; - } else if constexpr (base::is_same_v) { - out = text; - return true; } else if constexpr (base::is_integral_v) { i64 v = 0; if (!base::ParseInteger(text, v, /*base_radix=*/0)) return false; @@ -124,6 +123,18 @@ class BASE_EXPORT OptionBase : public InitChain { // programmatic default. Declare options at namespace scope, not as // function-local statics: InitOptionsFromEnv() can only populate options that // were already constructed (and thus registered) by the time it runs. +namespace detail { +// The buffer a const char* option keeps its value in. Empty for every other T, +// and empty bases/members cost nothing here, so an Option is the same +// size it always was. +template +struct OwnedText {}; +template <> +struct OwnedText { + StringU8 text; +}; +} // namespace detail + template class Option : public OptionBase { public: @@ -149,14 +160,32 @@ class Option : public OptionBase { protected: bool ParseFromString(const char* text) override { - return detail::ParseOption(text, value_); + if constexpr (base::is_same_v) { + // A string option owns its value. What SetFromString is handed is + // whatever its caller had at the time -- an environment variable read + // into a local, one line of a config file, a name off a command line -- + // and none of those are still there when the option is next read, so + // pointing at the caller's buffer is a use-after-free waiting for the + // first reader. + if (!text) return false; + owned_.text = reinterpret_cast(text); + value_ = reinterpret_cast(owned_.text.c_str()); + return true; + } else { + return detail::ParseOption(text, value_); + } } - void RestoreDefault() override { value_ = default_; } + void RestoreDefault() override { + value_ = default_; + // The default is a literal the option does not own; drop what it did. + if constexpr (base::is_same_v) owned_.text.clear(); + } private: T value_; const T default_; + [[no_unique_address]] detail::OwnedText owned_; }; // Populate every registered option that names an environment variable from the diff --git a/base/option_test.cc b/base/option_test.cc new file mode 100644 index 00000000..496733a6 --- /dev/null +++ b/base/option_test.cc @@ -0,0 +1,86 @@ +// Copyright (C) 2026 Vincent Hengel. +// For licensing information see LICENSE at the root of this distribution. + +#include +#include +#include + +#include + +namespace { + +base::Option kFlag{"opt.flag", false, "BASE_OPT_FLAG", "a boolean"}; +base::Option kCount{"opt.count", 7, "BASE_OPT_COUNT", "a number"}; +base::Option kScale{"opt.scale", 1.0f, "BASE_OPT_SCALE"}; +base::Option kName{"opt.name", "default", "BASE_OPT_NAME"}; + +class OptionTest : public ::testing::Test { + protected: + void SetUp() override { TearDown(); } + void TearDown() override { + kFlag.Reset(); + kCount.Reset(); + kScale.Reset(); + kName.Reset(); + base::DeleteEnvironmentVariable(u8"BASE_OPT_FLAG"); + base::DeleteEnvironmentVariable(u8"BASE_OPT_COUNT"); + base::DeleteEnvironmentVariable(u8"BASE_OPT_SCALE"); + base::DeleteEnvironmentVariable(u8"BASE_OPT_NAME"); + } +}; + +TEST_F(OptionTest, ParsesEachType) { + EXPECT_TRUE(kFlag.SetFromString("on")); + EXPECT_TRUE(kCount.SetFromString("128")); + EXPECT_TRUE(kScale.SetFromString("0.25")); + EXPECT_TRUE(kFlag.get()); + EXPECT_EQ(kCount.get(), 128); + EXPECT_FLOAT_EQ(kScale.get(), 0.25f); +} + +// The whole reason a string option copies: SetFromString is handed whatever +// its caller had at the time, and the option is read long after that is gone. +TEST_F(OptionTest, StringValueOutlivesTheCallersBuffer) { + { + base::String text("from-a-temporary"); + ASSERT_TRUE(kName.SetFromString(text.c_str())); + } + EXPECT_STREQ(kName.get(), "from-a-temporary"); +} + +// InitOptionsFromEnv reads each variable into a local, which is the same trap +// one call further out. +TEST_F(OptionTest, EnvironmentOverrideOutlivesTheRead) { + ASSERT_TRUE(base::SetEnvironmentVariable(u8"BASE_OPT_NAME", u8"from-the-env")); + ASSERT_TRUE(base::SetEnvironmentVariable(u8"BASE_OPT_COUNT", u8"42")); + + EXPECT_GE(base::InitOptionsFromEnv(), 2u); + EXPECT_STREQ(kName.get(), "from-the-env"); + EXPECT_EQ(kCount.get(), 42); + EXPECT_TRUE(kName.overridden()); +} + +TEST_F(OptionTest, ResetDropsTheOwnedString) { + { + base::String text("temporary"); + ASSERT_TRUE(kName.SetFromString(text.c_str())); + } + kName.Reset(); + EXPECT_STREQ(kName.get(), "default"); + EXPECT_FALSE(kName.overridden()); +} + +TEST_F(OptionTest, RejectsWhatDoesNotParse) { + ASSERT_TRUE(kCount.SetFromString("9")); + EXPECT_FALSE(kCount.SetFromString("not-a-number")); + EXPECT_EQ(kCount.get(), 9); // a bad value leaves the old one alone + EXPECT_FALSE(kName.SetFromString(nullptr)); +} + +// A string option pays for its buffer; nothing else should. +TEST_F(OptionTest, NonStringOptionsCarryNoStorage) { + EXPECT_EQ(sizeof(base::Option), sizeof(base::Option)); + EXPECT_LT(sizeof(base::Option), sizeof(base::Option)); +} + +} // namespace