From c36b7781589d139a420b9deda02cbebfc2ee629b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 30 Oct 2025 21:07:23 -0500 Subject: [PATCH] safety --- esphome/components/climate/climate.cpp | 46 ++++++++++++++++++--- esphome/components/climate/climate.h | 30 +++++++++++++- esphome/components/climate/climate_traits.h | 46 +++++++++++++++++++-- 3 files changed, 112 insertions(+), 10 deletions(-) diff --git a/esphome/components/climate/climate.cpp b/esphome/components/climate/climate.cpp index ebc9e466e0..e596582de8 100644 --- a/esphome/components/climate/climate.cpp +++ b/esphome/components/climate/climate.cpp @@ -593,8 +593,25 @@ void ClimateDeviceRestoreState::apply(Climate *climate) { climate->publish_state(); } -// Template helper for setting primary modes with mutual exclusion -// Clears custom pointer and sets primary optional value +/** Template helper for setting primary modes (fan_mode, preset) with mutual exclusion. + * + * Climate devices have mutually exclusive mode pairs: + * - fan_mode (enum) vs custom_fan_mode_ (const char*) + * - preset (enum) vs custom_preset_ (const char*) + * + * Only one mode in each pair can be active at a time. This helper ensures setting a primary + * mode automatically clears its corresponding custom mode. + * + * Example state transitions: + * Before: custom_fan_mode_="Turbo", fan_mode=nullopt + * Call: set_fan_mode_(CLIMATE_FAN_HIGH) + * After: custom_fan_mode_=nullptr, fan_mode=CLIMATE_FAN_HIGH + * + * @param primary The primary mode optional (fan_mode or preset) + * @param custom_ptr Reference to the custom mode pointer (custom_fan_mode_ or custom_preset_) + * @param value The new primary mode value to set + * @return true if state changed, false if already set to this value + */ template bool set_primary_mode(optional &primary, const char *&custom_ptr, T value) { // Clear the custom mode (mutual exclusion) bool changed = custom_ptr != nullptr; @@ -607,15 +624,34 @@ template bool set_primary_mode(optional &primary, const char *&cu return false; } -// Template helper for setting custom modes with mutual exclusion -// Takes pre-computed values: the found pointer from traits and whether custom mode is currently set +/** Template helper for setting custom modes (custom_fan_mode_, custom_preset_) with mutual exclusion. + * + * This helper ensures setting a custom mode automatically clears its corresponding primary mode. + * It also validates that the custom mode exists in the device's supported modes (lifetime safety). + * + * Example state transitions: + * Before: fan_mode=CLIMATE_FAN_HIGH, custom_fan_mode_=nullptr + * Call: set_custom_fan_mode_("Turbo") + * After: fan_mode=nullopt, custom_fan_mode_="Turbo" (pointer from traits) + * + * Lifetime Safety: + * - found_ptr must come from traits.find_custom_*_mode_() + * - Only pointers found in traits are stored, ensuring they remain valid + * - Prevents dangling pointers from temporary strings + * + * @param custom_ptr Reference to the custom mode pointer to set + * @param primary The primary mode optional to clear + * @param found_ptr The validated pointer from traits (nullptr if not found) + * @param has_custom Whether a custom mode is currently active + * @return true if state changed, false otherwise + */ template bool set_custom_mode(const char *&custom_ptr, optional &primary, const char *found_ptr, bool has_custom) { if (found_ptr != nullptr) { // Clear the primary mode (mutual exclusion) bool changed = primary.has_value(); primary.reset(); - // Set the custom mode + // Set the custom mode (pointer is validated by caller from traits) if (changed || custom_ptr != found_ptr) { custom_ptr = found_ptr; return true; diff --git a/esphome/components/climate/climate.h b/esphome/components/climate/climate.h index c36625b2ae..c6cd7005c5 100644 --- a/esphome/components/climate/climate.h +++ b/esphome/components/climate/climate.h @@ -325,10 +325,36 @@ class Climate : public EntityBase { optional visual_min_humidity_override_{}; optional visual_max_humidity_override_{}; - /// The active custom fan mode of the climate device (protected - use get_custom_fan_mode() or setters). + /** The active custom fan mode of the climate device. + * + * PROTECTED ACCESS: External components must use get_custom_fan_mode() for read access. + * Derived climate classes must use set_custom_fan_mode_() / clear_custom_fan_mode_() to modify. + * + * POINTER LIFETIME SAFETY: + * This pointer MUST always point to an entry in the traits.supported_custom_fan_modes_ vector, + * or be nullptr. The protected setter set_custom_fan_mode_() enforces this by calling + * traits.find_custom_fan_mode_() to validate and obtain the correct pointer. + * + * Never assign directly - always use setters: + * this->set_custom_fan_mode_("Turbo"); // ✓ Safe - validates against traits + * this->custom_fan_mode_ = "Turbo"; // ✗ UNSAFE - may create dangling pointer + */ const char *custom_fan_mode_{nullptr}; - /// The active custom preset mode of the climate device (protected - use get_custom_preset() or setters). + /** The active custom preset mode of the climate device. + * + * PROTECTED ACCESS: External components must use get_custom_preset() for read access. + * Derived climate classes must use set_custom_preset_() / clear_custom_preset_() to modify. + * + * POINTER LIFETIME SAFETY: + * This pointer MUST always point to an entry in the traits.supported_custom_presets_ vector, + * or be nullptr. The protected setter set_custom_preset_() enforces this by calling + * traits.find_custom_preset_() to validate and obtain the correct pointer. + * + * Never assign directly - always use setters: + * this->set_custom_preset_("Eco"); // ✓ Safe - validates against traits + * this->custom_preset_ = "Eco"; // ✗ UNSAFE - may create dangling pointer + */ const char *custom_preset_{nullptr}; }; diff --git a/esphome/components/climate/climate_traits.h b/esphome/components/climate/climate_traits.h index cbd9d1dbf4..14dcbcff6c 100644 --- a/esphome/components/climate/climate_traits.h +++ b/esphome/components/climate/climate_traits.h @@ -157,6 +157,11 @@ class ClimateTraits { template void set_supported_custom_fan_modes(const char *const (&modes)[N]) { this->supported_custom_fan_modes_.assign(modes, modes + N); } + + // Deleted overloads to catch incorrect std::string usage at compile time with clear error messages + void set_supported_custom_fan_modes(const std::vector &modes) = delete; + void set_supported_custom_fan_modes(std::initializer_list modes) = delete; + const std::vector &get_supported_custom_fan_modes() const { return this->supported_custom_fan_modes_; } bool supports_custom_fan_mode(const char *custom_fan_mode) const { return vector_contains(this->supported_custom_fan_modes_, custom_fan_mode); @@ -180,6 +185,11 @@ class ClimateTraits { template void set_supported_custom_presets(const char *const (&presets)[N]) { this->supported_custom_presets_.assign(presets, presets + N); } + + // Deleted overloads to catch incorrect std::string usage at compile time with clear error messages + void set_supported_custom_presets(const std::vector &presets) = delete; + void set_supported_custom_presets(std::initializer_list presets) = delete; + const std::vector &get_supported_custom_presets() const { return this->supported_custom_presets_; } bool supports_custom_preset(const char *custom_preset) const { return vector_contains(this->supported_custom_presets_, custom_preset); @@ -269,9 +279,39 @@ class ClimateTraits { climate::ClimateFanModeMask supported_fan_modes_; climate::ClimateSwingModeMask supported_swing_modes_; climate::ClimatePresetMask supported_presets_; - // Store const char* pointers to avoid std::string overhead - // Pointers must remain valid for traits lifetime (typically string literals in rodata, - // or pointers to strings with sufficient lifetime like member variables) + + /** Custom mode storage using const char* pointers to eliminate std::string overhead. + * + * POINTER LIFETIME SAFETY REQUIREMENTS: + * Pointers stored here MUST remain valid for the entire lifetime of the ClimateTraits object. + * This is guaranteed when pointers point to: + * + * 1. String literals (rodata section, valid for program lifetime): + * traits.set_supported_custom_fan_modes({"Turbo", "Silent"}); + * + * 2. Static const data (valid for program lifetime): + * static const char* PRESET_ECO = "Eco"; + * traits.set_supported_custom_presets({PRESET_ECO}); + * + * 3. Member variables with sufficient lifetime: + * class MyClimate { + * std::vector custom_presets_; // Lives as long as component + * ClimateTraits traits() { + * // Extract from map keys that live as long as the component + * for (const auto& [name, config] : preset_map_) { + * custom_presets_.push_back(name.c_str()); + * } + * traits.set_supported_custom_presets(custom_presets_); + * } + * }; + * + * UNSAFE PATTERNS TO AVOID: + * std::string temp = "Mode"; + * traits.set_supported_custom_fan_modes({temp.c_str()}); // DANGLING POINTER! + * + * Protected setters in Climate class automatically validate pointers against these + * vectors, ensuring only safe pointers are stored in device state. + */ std::vector supported_custom_fan_modes_; std::vector supported_custom_presets_; };