From a28a6f7d934a905c907763fda31098a9616c5eec Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 17:29:34 -1000 Subject: [PATCH] [http_request] Fix data race on update_info_ strings in update task The update task runs in a FreeRTOS thread on ESP32 and was writing directly to update_info_ std::string fields while the main loop reads them via API (as StringRef pointers into the string buffer), MQTT, web server, and Prometheus. This is undefined behavior that can cause use-after-free crashes when a string reallocation invalidates a StringRef held by the API serialization path. Move all update_info_ and state_ writes into the existing defer() callback so they execute on the main loop. The task now accumulates results in a local UpdateInfo struct and moves it into update_info_ in the deferred callback, eliminating the cross-thread data race with zero steady-state memory overhead. --- .../update/http_request_update.cpp | 140 ++++++++++-------- .../http_request/update/http_request_update.h | 3 + 2 files changed, 85 insertions(+), 58 deletions(-) diff --git a/esphome/components/http_request/update/http_request_update.cpp b/esphome/components/http_request/update/http_request_update.cpp index c40590af955..dc80ebc8f05 100644 --- a/esphome/components/http_request/update/http_request_update.cpp +++ b/esphome/components/http_request/update/http_request_update.cpp @@ -74,42 +74,41 @@ void HttpRequestUpdate::update() { #endif } -void HttpRequestUpdate::update_task(void *params) { - HttpRequestUpdate *this_update = (HttpRequestUpdate *) params; - - auto container = this_update->request_parent_->get(this_update->source_url_); +// Fetch and parse the update manifest. Returns a heap-allocated UpdateInfo on success +// (caller takes ownership), or nullptr on failure (with error_str set to the error message). +// Separated from update_task so that all code paths converge at a single defer() call, +// ensuring the allocated UpdateInfo is always passed to apply_manifest_result_main_loop_ +// which deletes it — preventing leaks regardless of which error path is taken. +update::UpdateInfo *HttpRequestUpdate::fetch_manifest_(const LogString *&error_str) { + auto container = this->request_parent_->get(this->source_url_); if (container == nullptr || container->status_code != HTTP_STATUS_OK) { - ESP_LOGE(TAG, "Failed to fetch manifest from %s", this_update->source_url_.c_str()); - // Defer to main loop to avoid race condition on component_state_ read-modify-write - this_update->defer([this_update]() { this_update->status_set_error(LOG_STR("Failed to fetch manifest")); }); - UPDATE_RETURN; + ESP_LOGE(TAG, "Failed to fetch manifest from %s", this->source_url_.c_str()); + error_str = LOG_STR("Failed to fetch manifest"); + return nullptr; } RAMAllocator allocator; uint8_t *data = allocator.allocate(container->content_length); if (data == nullptr) { ESP_LOGE(TAG, "Failed to allocate %zu bytes for manifest", container->content_length); - // Defer to main loop to avoid race condition on component_state_ read-modify-write - this_update->defer( - [this_update]() { this_update->status_set_error(LOG_STR("Failed to allocate memory for manifest")); }); + error_str = LOG_STR("Failed to allocate memory for manifest"); container->end(); - UPDATE_RETURN; + return nullptr; } auto read_result = http_read_fully(container.get(), data, container->content_length, MAX_READ_SIZE, - this_update->request_parent_->get_timeout()); + this->request_parent_->get_timeout()); if (read_result.status != HttpReadStatus::OK) { if (read_result.status == HttpReadStatus::TIMEOUT) { ESP_LOGE(TAG, "Timeout reading manifest"); } else { ESP_LOGE(TAG, "Error reading manifest: %d", read_result.error_code); } - // Defer to main loop to avoid race condition on component_state_ read-modify-write - this_update->defer([this_update]() { this_update->status_set_error(LOG_STR("Failed to read manifest")); }); + error_str = LOG_STR("Failed to read manifest"); allocator.deallocate(data, container->content_length); container->end(); - UPDATE_RETURN; + return nullptr; } size_t read_index = container->get_bytes_read(); size_t content_length = container->content_length; @@ -117,16 +116,18 @@ void HttpRequestUpdate::update_task(void *params) { container->end(); container.reset(); // Release ownership of the container's shared_ptr + auto *info = new update::UpdateInfo(); + bool valid = false; { // Scope to ensure JsonDocument is destroyed before deallocating buffer - valid = json::parse_json(data, read_index, [this_update](JsonObject root) -> bool { + valid = json::parse_json(data, read_index, [info](JsonObject root) -> bool { if (!root[ESPHOME_F("name")].is() || !root[ESPHOME_F("version")].is() || !root[ESPHOME_F("builds")].is()) { ESP_LOGE(TAG, "Manifest does not contain required fields"); return false; } - this_update->update_info_.title = root[ESPHOME_F("name")].as(); - this_update->update_info_.latest_version = root[ESPHOME_F("version")].as(); + info->title = root[ESPHOME_F("name")].as(); + info->latest_version = root[ESPHOME_F("version")].as(); auto builds_array = root[ESPHOME_F("builds")].as(); for (auto build : builds_array) { @@ -144,13 +145,13 @@ void HttpRequestUpdate::update_task(void *params) { ESP_LOGE(TAG, "Manifest does not contain required fields"); return false; } - this_update->update_info_.firmware_url = ota[ESPHOME_F("path")].as(); - this_update->update_info_.md5 = ota[ESPHOME_F("md5")].as(); + info->firmware_url = ota[ESPHOME_F("path")].as(); + info->md5 = ota[ESPHOME_F("md5")].as(); if (ota[ESPHOME_F("summary")].is()) - this_update->update_info_.summary = ota[ESPHOME_F("summary")].as(); + info->summary = ota[ESPHOME_F("summary")].as(); if (ota[ESPHOME_F("release_url")].is()) - this_update->update_info_.release_url = ota[ESPHOME_F("release_url")].as(); + info->release_url = ota[ESPHOME_F("release_url")].as(); return true; } @@ -161,62 +162,85 @@ void HttpRequestUpdate::update_task(void *params) { allocator.deallocate(data, content_length); if (!valid) { - ESP_LOGE(TAG, "Failed to parse JSON from %s", this_update->source_url_.c_str()); - // Defer to main loop to avoid race condition on component_state_ read-modify-write - this_update->defer([this_update]() { this_update->status_set_error(LOG_STR("Failed to parse manifest JSON")); }); - UPDATE_RETURN; + delete info; + ESP_LOGE(TAG, "Failed to parse JSON from %s", this->source_url_.c_str()); + error_str = LOG_STR("Failed to parse manifest JSON"); + return nullptr; } - // Merge source_url_ and this_update->update_info_.firmware_url - if (this_update->update_info_.firmware_url.find("http") == std::string::npos) { - std::string path = this_update->update_info_.firmware_url; + // Merge source_url_ and firmware_url + if (info->firmware_url.find("http") == std::string::npos) { + std::string path = info->firmware_url; if (path[0] == '/') { - std::string domain = this_update->source_url_.substr(0, this_update->source_url_.find('/', 8)); - this_update->update_info_.firmware_url = domain + path; + std::string domain = this->source_url_.substr(0, this->source_url_.find('/', 8)); + info->firmware_url = domain + path; } else { - std::string domain = this_update->source_url_.substr(0, this_update->source_url_.rfind('/') + 1); - this_update->update_info_.firmware_url = domain + path; + std::string domain = this->source_url_.substr(0, this->source_url_.rfind('/') + 1); + info->firmware_url = domain + path; } } #ifdef ESPHOME_PROJECT_VERSION - this_update->update_info_.current_version = ESPHOME_PROJECT_VERSION; + info->current_version = ESPHOME_PROJECT_VERSION; #else - this_update->update_info_.current_version = ESPHOME_VERSION; + info->current_version = ESPHOME_VERSION; #endif + return info; +} + +void HttpRequestUpdate::update_task(void *params) { + HttpRequestUpdate *this_update = (HttpRequestUpdate *) params; + + const LogString *error_str = nullptr; + auto *info = this_update->fetch_manifest_(error_str); + + update::UpdateState new_state = update::UPDATE_STATE_UNKNOWN; bool trigger_update_available = false; - if (this_update->update_info_.latest_version.empty() || - this_update->update_info_.latest_version == this_update->update_info_.current_version) { - this_update->state_ = update::UPDATE_STATE_NO_UPDATE; - } else { - if (this_update->state_ != update::UPDATE_STATE_AVAILABLE) { - trigger_update_available = true; + if (info != nullptr) { + if (info->latest_version.empty() || info->latest_version == info->current_version) { + new_state = update::UPDATE_STATE_NO_UPDATE; + } else { + new_state = update::UPDATE_STATE_AVAILABLE; + if (this_update->state_ != update::UPDATE_STATE_AVAILABLE) { + trigger_update_available = true; + } } - this_update->state_ = update::UPDATE_STATE_AVAILABLE; } - // Defer to main loop to ensure thread-safe execution of: - // - status_clear_error() performs non-atomic read-modify-write on component_state_ - // - publish_state() triggers API callbacks that write to the shared protobuf buffer - // which can be corrupted if accessed concurrently from task and main loop threads - // - update_available trigger to ensure consistent state when the trigger fires - this_update->defer([this_update, trigger_update_available]() { - this_update->update_info_.has_progress = false; - this_update->update_info_.progress = 0.0f; - - this_update->status_clear_error(); - this_update->publish_state(); - - if (trigger_update_available) { - this_update->get_update_available_trigger()->trigger(this_update->update_info_); - } + // Defer to the main loop so all update_info_ and state_ writes happen on the + // same thread as readers (API, MQTT, web server). This is a single defer for + // both success and error paths to avoid multiple std::function instantiations. + // Ownership of info transfers to apply_manifest_result_main_loop_. + this_update->defer([this_update, trigger_update_available, new_state, error_str, info]() { + this_update->apply_manifest_result_main_loop_(info, error_str, new_state, trigger_update_available); }); UPDATE_RETURN; } +void HttpRequestUpdate::apply_manifest_result_main_loop_(update::UpdateInfo *info, const LogString *error_str, + update::UpdateState new_state, bool trigger_update_available) { + if (error_str != nullptr) { + delete info; + this->status_set_error(error_str); + return; + } + this->update_info_ = std::move(*info); + this->update_info_.has_progress = false; + this->update_info_.progress = 0.0f; + this->state_ = new_state; + delete info; + + this->status_clear_error(); + this->publish_state(); + + if (trigger_update_available) { + this->get_update_available_trigger()->trigger(this->update_info_); + } +} + void HttpRequestUpdate::perform(bool force) { if (this->state_ != update::UPDATE_STATE_AVAILABLE && !force) { return; diff --git a/esphome/components/http_request/update/http_request_update.h b/esphome/components/http_request/update/http_request_update.h index b8350346f9e..8b6fb2443f3 100644 --- a/esphome/components/http_request/update/http_request_update.h +++ b/esphome/components/http_request/update/http_request_update.h @@ -37,6 +37,9 @@ class HttpRequestUpdate final : public update::UpdateEntity, public PollingCompo std::string source_url_; static void update_task(void *params); + update::UpdateInfo *fetch_manifest_(const LogString *&error_str); + void apply_manifest_result_main_loop_(update::UpdateInfo *info, const LogString *error_str, + update::UpdateState new_state, bool trigger_update_available); #ifdef USE_ESP32 TaskHandle_t update_task_handle_{nullptr}; #endif