From 9b6facb20d5461dfaf47fd3993a7b4601f082c34 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 8 Sep 2026 15:46:17 -0400 Subject: [PATCH] [core] Fix use-after-free when deleting a running StaticTask (#19048) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../micro_wake_word/micro_wake_word.cpp | 4 +-- .../mixer/speaker/mixer_speaker.cpp | 4 +-- .../resampler/speaker/resampler_speaker.cpp | 4 +-- .../speaker/media_player/audio_pipeline.cpp | 11 +++++-- esphome/core/static_task.cpp | 30 ++++++++++++++----- esphome/core/static_task.h | 17 +++++++---- 6 files changed, 50 insertions(+), 20 deletions(-) diff --git a/esphome/components/micro_wake_word/micro_wake_word.cpp b/esphome/components/micro_wake_word/micro_wake_word.cpp index 3dadb78077..cebfe8e791 100644 --- a/esphome/components/micro_wake_word/micro_wake_word.cpp +++ b/esphome/components/micro_wake_word/micro_wake_word.cpp @@ -446,9 +446,9 @@ void MicroWakeWord::loop() { xEventGroupClearBits(this->event_group_, EventGroupBits::TASK_STOPPING); } - if ((event_group_bits & EventGroupBits::TASK_STOPPED)) { + // Retries on a subsequent loop if the task is still running on the other core + if ((event_group_bits & EventGroupBits::TASK_STOPPED) && this->inference_task_.deallocate()) { ESP_LOGD(TAG, "Inference task is finished, freeing task resources"); - this->inference_task_.deallocate(); xEventGroupClearBits(this->event_group_, ALL_BITS); xQueueReset(this->detection_queue_); this->set_state_(State::STOPPED); diff --git a/esphome/components/mixer/speaker/mixer_speaker.cpp b/esphome/components/mixer/speaker/mixer_speaker.cpp index 6128dc3767..0b79010773 100644 --- a/esphome/components/mixer/speaker/mixer_speaker.cpp +++ b/esphome/components/mixer/speaker/mixer_speaker.cpp @@ -382,8 +382,8 @@ void MixerSpeaker::loop() { ESP_LOGV(TAG, "Stopping"); xEventGroupClearBits(this->event_group_, MIXER_TASK_STATE_STOPPING); } - if (event_group_bits & MIXER_TASK_STATE_STOPPED) { - this->task_.deallocate(); + // Retries on a subsequent loop if the task is still running on the other core + if ((event_group_bits & MIXER_TASK_STATE_STOPPED) && this->task_.deallocate()) { ESP_LOGD(TAG, "Stopped"); xEventGroupClearBits(this->event_group_, MIXER_TASK_ALL_BITS); this->all_stopped_since_ms_ = 0; diff --git a/esphome/components/resampler/speaker/resampler_speaker.cpp b/esphome/components/resampler/speaker/resampler_speaker.cpp index f1ebd180cc..edda00ae06 100644 --- a/esphome/components/resampler/speaker/resampler_speaker.cpp +++ b/esphome/components/resampler/speaker/resampler_speaker.cpp @@ -153,8 +153,8 @@ void ResamplerSpeaker::loop() { ESP_LOGV(TAG, "Stopping"); xEventGroupClearBits(this->event_group_, ResamplingEventGroupBits::STATE_STOPPING); } - if (event_group_bits & ResamplingEventGroupBits::STATE_STOPPED) { - this->task_.deallocate(); + // Retries on a subsequent loop if the task is still running on the other core + if ((event_group_bits & ResamplingEventGroupBits::STATE_STOPPED) && this->task_.deallocate()) { ESP_LOGD(TAG, "Stopped"); xEventGroupClearBits(this->event_group_, ResamplingEventGroupBits::ALL_BITS); } diff --git a/esphome/components/speaker/media_player/audio_pipeline.cpp b/esphome/components/speaker/media_player/audio_pipeline.cpp index 010f0c50b3..c286a9d7d6 100644 --- a/esphome/components/speaker/media_player/audio_pipeline.cpp +++ b/esphome/components/speaker/media_player/audio_pipeline.cpp @@ -202,8 +202,15 @@ AudioPipelineState AudioPipeline::process_state() { if (!this->is_playing_) { // The tasks have been stopped for two ``process_state`` calls in a row, so delete the tasks if (this->read_task_.is_created() || this->decode_task_.is_created()) { - this->read_task_.deallocate(); - this->decode_task_.deallocate(); + // Both are attempted every time; a task that is still running on the other core is freed by a + // subsequent call, and freeing an already freed task succeeds without doing anything + bool read_task_freed = this->read_task_.deallocate(); + bool decode_task_freed = this->decode_task_.deallocate(); + if (!read_task_freed || !decode_task_freed) { + // A task is still running on the other core, so keep the pipeline in its current state and try + // again on the next call + return AudioPipelineState::PLAYING; + } if (this->hard_stop_) { // Stop command was sent, so immediately end the playback this->speaker_->stop(); diff --git a/esphome/core/static_task.cpp b/esphome/core/static_task.cpp index 4cfead44c2..4301108315 100644 --- a/esphome/core/static_task.cpp +++ b/esphome/core/static_task.cpp @@ -40,16 +40,31 @@ bool StaticTask::create(TaskFunction_t fn, const char *name, uint32_t stack_size return true; } -void StaticTask::destroy() { - if (this->handle_ != nullptr) { - TaskHandle_t handle = this->handle_; - this->handle_ = nullptr; - vTaskDelete(handle); +bool StaticTask::destroy() { + if (this->handle_ == nullptr) { + return true; } + + // Suspending takes the task off the ready and event lists, so nothing can schedule it again. It only asks + // the other core to yield though, so the task may still be running on it for a moment. + vTaskSuspend(this->handle_); + if (eTaskGetState(this->handle_) != eSuspended) { + // The task is still running on the other core and using its stack. Deleting it now would only put it on + // the termination list and return, so the caller has to try again once it has been swapped out. + return false; + } + + // The task cannot run again, so the delete completes right away instead of being left to the idle task. + TaskHandle_t handle = this->handle_; + this->handle_ = nullptr; + vTaskDelete(handle); + return true; } -void StaticTask::deallocate() { - this->destroy(); +bool StaticTask::deallocate() { + if (!this->destroy()) { + return false; + } if (this->stack_buffer_ != nullptr) { RAMAllocator allocator(this->use_psram_ ? RAMAllocator::ALLOC_EXTERNAL : RAMAllocator::ALLOC_INTERNAL); @@ -57,6 +72,7 @@ void StaticTask::deallocate() { this->stack_buffer_ = nullptr; this->stack_size_ = 0; } + return true; } } // namespace esphome diff --git a/esphome/core/static_task.h b/esphome/core/static_task.h index 5fd5b38f9e..e2996abeda 100644 --- a/esphome/core/static_task.h +++ b/esphome/core/static_task.h @@ -11,6 +11,7 @@ namespace esphome { /** Helper for FreeRTOS static task management. * Bundles TaskHandle_t, StaticTask_t, and the stack buffer into one object with create/destroy methods. + * Call destroy() and deallocate() from another task: a task cannot free the stack it is still running on. */ class StaticTask { public: @@ -23,7 +24,7 @@ class StaticTask { /// @brief Allocate stack and create task. /// @param fn Task function /// @param name Task name (for debug) - /// @param stack_size Stack size in StackType_t words + /// @param stack_size Stack size in bytes (StackType_t is a byte on ESP-IDF) /// @param param Parameter passed to task function /// @param priority FreeRTOS task priority /// @param use_psram If true, allocate stack in PSRAM; otherwise internal RAM @@ -31,11 +32,17 @@ class StaticTask { bool create(TaskFunction_t fn, const char *name, uint32_t stack_size, void *param, UBaseType_t priority, bool use_psram); - /// @brief Delete the task but keep the stack buffer allocated for reuse by a subsequent create() call. - void destroy(); + /// @brief Delete the task, keeping the stack buffer allocated for reuse by a subsequent create() call. + /// The task must have finished its work and parked itself, either suspended or blocked indefinitely: it is + /// suspended here so that it cannot be scheduled again, and it is given no chance to clean up. + /// @return true if the task was deleted; false if it is still running on another core, in which case the + /// caller should try again later. + bool destroy(); - /// @brief Delete the task (if running) and free the stack buffer. - void deallocate(); + /// @brief Delete the task (if created) and free the stack buffer. + /// @return true if the stack buffer was freed; false if the task is still running on another core, in + /// which case the caller should try again later. + bool deallocate(); protected: TaskHandle_t handle_{nullptr};