diff --git a/esphome/components/sprinkler/sprinkler.cpp b/esphome/components/sprinkler/sprinkler.cpp index 7676e174688f..c6c2c340a9d6 100644 --- a/esphome/components/sprinkler/sprinkler.cpp +++ b/esphome/components/sprinkler/sprinkler.cpp @@ -431,13 +431,12 @@ void Sprinkler::add_valve(SprinklerControllerSwitch *valve_sw, SprinklerControll new_valve->valve_turn_off_automation = make_unique>(new_valve->controller_switch->get_turn_off_trigger()); - new_valve->valve_shutdown_action = make_unique>(this); - new_valve->valve_turn_off_automation->add_actions({new_valve->valve_shutdown_action.get()}); + new_valve->valve_turn_off_automation->add_actions({new sprinkler::ShutdownAction<>(this)}); new_valve->valve_turn_on_automation = make_unique>(new_valve->controller_switch->get_turn_on_trigger()); - new_valve->valve_resumeorstart_action = make_unique>(this); - new_valve->valve_resumeorstart_action->set_valve_to_start(new_valve_number); - new_valve->valve_turn_on_automation->add_actions({new_valve->valve_resumeorstart_action.get()}); + auto *valve_resumeorstart_action = new sprinkler::StartSingleValveAction<>(this); + valve_resumeorstart_action->set_valve_to_start(new_valve_number); + new_valve->valve_turn_on_automation->add_actions({valve_resumeorstart_action}); if (enable_sw != nullptr) { new_valve->enable_switch = enable_sw; @@ -458,12 +457,10 @@ void Sprinkler::set_controller_main_switch(SprinklerControllerSwitch *controller }); this->sprinkler_turn_off_automation_ = make_unique>(controller_switch->get_turn_off_trigger()); - this->sprinkler_shutdown_action_ = make_unique>(this); - this->sprinkler_turn_off_automation_->add_actions({sprinkler_shutdown_action_.get()}); + this->sprinkler_turn_off_automation_->add_actions({new sprinkler::ShutdownAction<>(this)}); this->sprinkler_turn_on_automation_ = make_unique>(controller_switch->get_turn_on_trigger()); - this->sprinkler_resumeorstart_action_ = make_unique>(this); - this->sprinkler_turn_on_automation_->add_actions({sprinkler_resumeorstart_action_.get()}); + this->sprinkler_turn_on_automation_->add_actions({new sprinkler::ResumeOrStartAction<>(this)}); } void Sprinkler::set_controller_auto_adv_switch(SprinklerControllerSwitch *auto_adv_switch) { @@ -482,8 +479,7 @@ void Sprinkler::set_controller_standby_switch(SprinklerControllerSwitch *standby this->standby_sw_ = standby_switch; this->sprinkler_standby_turn_on_automation_ = make_unique>(standby_switch->get_turn_on_trigger()); - this->sprinkler_standby_shutdown_action_ = make_unique>(this); - this->sprinkler_standby_turn_on_automation_->add_actions({sprinkler_standby_shutdown_action_.get()}); + this->sprinkler_standby_turn_on_automation_->add_actions({new sprinkler::ShutdownAction<>(this)}); } void Sprinkler::set_controller_multiplier_number(SprinklerControllerNumber *multiplier_number) { diff --git a/esphome/components/sprinkler/sprinkler.h b/esphome/components/sprinkler/sprinkler.h index c4a8b8aeb880..05823fbd6c8f 100644 --- a/esphome/components/sprinkler/sprinkler.h +++ b/esphome/components/sprinkler/sprinkler.h @@ -93,8 +93,7 @@ struct SprinklerValve { uint32_t run_duration; optional pump_switch_index; bool valve_cycle_complete; - std::unique_ptr> valve_shutdown_action; - std::unique_ptr> valve_resumeorstart_action; + // The shutdown / start actions belong to the automations' ActionLists; a second owning handle would double-free. std::unique_ptr> valve_turn_off_automation; std::unique_ptr> valve_turn_on_automation; }; @@ -598,10 +597,7 @@ class Sprinkler : public Component { SprinklerControllerNumber *multiplier_number_{nullptr}; SprinklerControllerNumber *repeat_number_{nullptr}; - std::unique_ptr> sprinkler_shutdown_action_; - std::unique_ptr> sprinkler_standby_shutdown_action_; - std::unique_ptr> sprinkler_resumeorstart_action_; - + // As in SprinklerValve: the actions belong to the automations below. std::unique_ptr> sprinkler_turn_off_automation_; std::unique_ptr> sprinkler_turn_on_automation_; std::unique_ptr> sprinkler_standby_turn_on_automation_; diff --git a/esphome/core/automation.h b/esphome/core/automation.h index 05fd07a4e345..e06e202b360e 100644 --- a/esphome/core/automation.h +++ b/esphome/core/automation.h @@ -123,6 +123,9 @@ template class TemplatableValue { */ template class Condition { public: + /// Virtual so an owner can delete a child through this base. + virtual ~Condition() = default; + /// Check whether this condition passes. This condition check must be instant, and not cause any delays. virtual bool check(Ts... x) = 0; @@ -177,6 +180,9 @@ template class ActionList; template class Action { public: + /// Virtual so the owning ActionList can delete a node through this base. + virtual ~Action() = default; + virtual void play_complex(Ts... x) { this->num_running_++; this->play(x...); @@ -242,6 +248,23 @@ template class Action { template class ActionList { public: + ActionList() = default; + + /// Owns its chain. Nothing reachable from a list that gets destroyed may be registered with App: Action and + /// Condition have no prepare_for_deletion() to decline with. + ~ActionList() { + Action *action = this->actions_begin_; + while (action != nullptr) { + Action *next = action->next_; + delete action; + action = next; + } + } + + // Owning, so a copy would free the same chain twice. + ActionList(const ActionList &) = delete; + ActionList &operator=(const ActionList &) = delete; + void add_action(Action *action) { if (this->actions_end_ == nullptr) { this->actions_begin_ = action; diff --git a/esphome/core/base_automation.h b/esphome/core/base_automation.h index f98591bc8533..44691cd8b75d 100644 --- a/esphome/core/base_automation.h +++ b/esphome/core/base_automation.h @@ -15,6 +15,11 @@ namespace esphome { template class AndCondition : public Condition { public: explicit AndCondition(const std::vector *> &conditions) : conditions_(conditions) {} + ~AndCondition() override { + for (auto *condition : this->conditions_) { + delete condition; + } + } bool check(Ts... x) override { for (auto *condition : this->conditions_) { if (!condition->check(x...)) @@ -31,6 +36,11 @@ template class AndCondition : public Condition { template class OrCondition : public Condition { public: explicit OrCondition(const std::vector *> &conditions) : conditions_(conditions) {} + ~OrCondition() override { + for (auto *condition : this->conditions_) { + delete condition; + } + } bool check(Ts... x) override { for (auto *condition : this->conditions_) { if (condition->check(x...)) @@ -47,6 +57,7 @@ template class OrCondition : public Condition { template class NotCondition : public Condition { public: explicit NotCondition(Condition *condition) : condition_(condition) {} + ~NotCondition() override { delete this->condition_; } bool check(Ts... x) override { return !this->condition_->check(x...); } protected: @@ -56,6 +67,11 @@ template class NotCondition : public Condition { template class XorCondition : public Condition { public: explicit XorCondition(const std::vector *> &conditions) : conditions_(conditions) {} + ~XorCondition() override { + for (auto *condition : this->conditions_) { + delete condition; + } + } bool check(Ts... x) override { size_t result = 0; for (auto *condition : this->conditions_) { @@ -81,6 +97,7 @@ template class LambdaCondition : public Condition { template class ForCondition : public Condition, public Component { public: explicit ForCondition(Condition<> *condition) : condition_(condition) {} + ~ForCondition() override { delete this->condition_; } TEMPLATABLE_VALUE(uint32_t, time); @@ -167,6 +184,9 @@ template class DelayAction : public Action, public Compon public: explicit DelayAction() = default; + /// SchedulerItem keeps a raw Component*, and stop() does not run on every teardown path. + ~DelayAction() override { this->cancel_timeout("delay"); } + TEMPLATABLE_VALUE(uint32_t, delay) void play_complex(Ts... x) override { @@ -204,6 +224,8 @@ template class LambdaAction : public Action { template class IfAction : public Action { public: explicit IfAction(Condition *condition) : condition_(condition) {} + /// then_ / else_ are by-value ActionLists and free themselves. + ~IfAction() override { delete this->condition_; } void add_then(const std::vector *> &actions) { this->then_.add_actions(actions); @@ -250,6 +272,7 @@ template class IfAction : public Action { template class WhileAction : public Action { public: WhileAction(Condition *condition) : condition_(condition) {} + ~WhileAction() override { delete this->condition_; } void add_then(const std::vector *> &actions) { this->then_.add_actions(actions); @@ -333,6 +356,11 @@ template class RepeatAction : public Action { template class WaitUntilAction : public Action, public Component { public: WaitUntilAction(Condition *condition) : condition_(condition) {} + /// Same scheduler hazard as ~DelayAction, plus the owned condition. + ~WaitUntilAction() override { + this->cancel_timeout("timeout"); + delete this->condition_; + } TEMPLATABLE_VALUE(uint32_t, timeout_value) diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index 2500b99f512f..51ac1994482b 100644 --- a/esphome/core/component.cpp +++ b/esphome/core/component.cpp @@ -238,7 +238,7 @@ void IRAM_ATTR HOT Component::enable_loop_soon_any_context() { // 2. No read-modify-write operations that could be interrupted // 3. No memory allocation, object construction, or function calls // 4. IRAM_ATTR ensures code is in IRAM, not flash (required for ISR execution) - // 5. Components are never destroyed, so no use-after-free concerns + // 5. Components registered with App are never destroyed, so no use-after-free concerns // 6. App is guaranteed to be initialized before any ISR could fire // 7. Multiple ISR/thread calls are safe - just sets the same flags to true // 8. Race condition with main loop is handled by clearing flag before processing diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 402084f306d1..ea690bba1ec4 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -441,17 +441,12 @@ void HOT Scheduler::call(uint32_t now) { // Not reached timeout yet, done for this call break; } - // Don't run on failed components - if (item->component != nullptr && item->component->is_failed()) { - LockGuard guard{this->lock_}; - this->pop_raw_(); - continue; - } - // Check if item is marked for removal // This handles two cases: // 1. Item was marked for removal after cleanup_() but before we got here // 2. Item is marked for removal but wasn't at the front of the heap during cleanup_() + // Must precede the is_failed() check below, as should_skip_item_() orders them: a merely marked item keeps a + // raw Component * that may already be freed. #ifdef ESPHOME_THREAD_MULTI_NO_ATOMICS // Multi-threaded platforms without atomics: must take lock to safely read remove flag { @@ -472,6 +467,13 @@ void HOT Scheduler::call(uint32_t now) { } #endif + // Don't run on failed components + if (item->component != nullptr && item->component->is_failed()) { + LockGuard guard{this->lock_}; + this->pop_raw_(); + continue; + } + #ifdef ESPHOME_DEBUG_SCHEDULER const char *item_name = item->get_name(); ESP_LOGV(TAG, "Running %s '%s/%s' with interval=%" PRIu32 " next_execution=%" PRIu64 " (now=%" PRIu64 ")",