From b2ffac272fc107b03b57c4f8045f29f7194579db Mon Sep 17 00:00:00 2001 From: Aleksey Zinchenko Date: Mon, 17 Aug 2026 13:07:49 +0300 Subject: [PATCH] [core] Free the action/condition tree when an Automation is destroyed ~Automation freed its triggers and nothing else. The action list, every action in it, and every condition hanging off those actions were leaked. Upstream that is harmless -- code-generated automations are new'ed once and never destroyed -- but the JetHome `automations` component rebuilds its runtime object graph on every save from the web editor, and on every remove/reset. core/automation.h gets virtual ~Action() and ~Condition() (deleting through those bases was UB), and an owning ~ActionList() that walks the actions_begin_/next_ chain. Automation::actions_ is a by-value member, so ~Automation now frees the whole tree without a change of its own. ActionList copy/assign are deleted: a copy would free the same chain twice. core/base_automation.h: the owners free what they hold -- IfAction and WhileAction their condition, ForCondition and WaitUntilAction theirs, and the And/Or/Xor/Not group conditions their children. Three hazards the destructors expose, handled here: - DelayAction and WaitUntilAction are Components that arm a scheduler timer against themselves, and Scheduler::SchedulerItem keeps a raw Component* it dereferences (is_failed()) while walking its heap. Freeing one with a timer pending is a use-after-free, not a stale callback. Both destructors cancel first; stop() alone is not enough, since stop_complex() only calls it while num_running_ != 0 and a runtime build can be torn down without any stop() at all. Both timer names are static strings, so both are cancellable. - Cancelling is only half of it. cancel_item_locked_() unlinks an item outright only when it is items_.back(); everywhere else it just marks it, leaving it in the heap still holding that raw Component*. The dispatch loop in call() then read item->component->is_failed() *before* testing the remove flag, so a cancelled item belonging to a freed component was still dereferenced -- and cleanup_() does not necessarily reach it first, since it pops only leading removed items and full compaction needs MAX_LOGICALLY_DELETED_ITEMS. The two checks are now in the same order Scheduler::should_skip_item_() has always used for the defer queue. As a side effect the to_remove_ counter no longer drifts when an item is both removed and owned by a failed component. - sprinkler held its shutdown / resume-or-start actions in unique_ptrs *and* handed the same raw pointers to an Automation's ActionList. With an owning ActionList that is a double free. The ActionList is now the sole owner and the redundant members are gone; nothing else read them. Dormant before this change (a Sprinkler is a codegen global whose Automations outlive the process), so this closes a latent bug rather than one that was firing. Every add_action / add_actions / add_then / add_else call site was audited for a second owner or a non-heap pointer; sprinkler was the only one. Codegen passes only new_Pvariable objects, each into exactly one list. A behavioural no-op for statically generated automations. Known limit, documented at ~ActionList rather than fixed: Trigger can decline deletion via prepare_for_deletion(), Action and Condition cannot, so "an Action/Condition owned by an ActionList must never be registered with App" is a call-site rule. Codegen registers DelayAction, WaitUntilAction and ForCondition, which is safe only because codegen automations are never destroyed. Co-Authored-By: Claude Opus 5 (1M context) --- esphome/components/sprinkler/sprinkler.cpp | 18 ++++++-------- esphome/components/sprinkler/sprinkler.h | 8 ++----- esphome/core/automation.h | 23 ++++++++++++++++++ esphome/core/base_automation.h | 28 ++++++++++++++++++++++ esphome/core/component.cpp | 2 +- esphome/core/scheduler.cpp | 16 +++++++------ 6 files changed, 70 insertions(+), 25 deletions(-) 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 ")",