sascha/esphome
1
0
Fork 0
mirror of https://github.com/esphome/esphome.git synced 2025-11-19 16:25:50 +00:00
Code Issues Releases Wiki Activity

[scheduler] Add defensive nullptr checks and explicit locking requirements

Browse Source
This commit is contained in:
J. Nick Koston
2025-11-18 11:31:01 -06:00
parent 33983b051b
commit d83a698398
2 changed files with 31 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
esphome/core/scheduler.cpp
View File

@@ -154,8 +154,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type
// For retries, check if there's a cancelled timeout first // For retries, check if there's a cancelled timeout first
if (is_retry && name_cstr != nullptr && type == SchedulerItem::TIMEOUT && if (is_retry && name_cstr != nullptr && type == SchedulerItem::TIMEOUT &&
(has_cancelled_timeout_in_container_(this->items_, component, name_cstr, /* match_retry= */ true) || (has_cancelled_timeout_in_container_locked_(this->items_, component, name_cstr, /* match_retry= */ true) ||
has_cancelled_timeout_in_container_(this->to_add_, component, name_cstr, /* match_retry= */ true))) { has_cancelled_timeout_in_container_locked_(this->to_add_, component, name_cstr, /* match_retry= */ true))) {
// Skip scheduling - the retry was cancelled // Skip scheduling - the retry was cancelled
#ifdef ESPHOME_DEBUG_SCHEDULER #ifdef ESPHOME_DEBUG_SCHEDULER
ESP_LOGD(TAG, "Skipping retry '%s' - found cancelled item", name_cstr); ESP_LOGD(TAG, "Skipping retry '%s' - found cancelled item", name_cstr);
@@ -556,7 +556,8 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c
#ifndef ESPHOME_THREAD_SINGLE #ifndef ESPHOME_THREAD_SINGLE
// Mark items in defer queue as cancelled (they'll be skipped when processed) // Mark items in defer queue as cancelled (they'll be skipped when processed)
if (type == SchedulerItem::TIMEOUT) { if (type == SchedulerItem::TIMEOUT) {
total_cancelled += this->mark_matching_items_removed_(this->defer_queue_, component, name_cstr, type, match_retry); total_cancelled +=
this->mark_matching_items_removed_locked_(this->defer_queue_, component, name_cstr, type, match_retry);
} }
#endif /* not ESPHOME_THREAD_SINGLE */ #endif /* not ESPHOME_THREAD_SINGLE */
@@ -565,19 +566,20 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c
// (removing the last element doesn't break heap structure) // (removing the last element doesn't break heap structure)
if (!this->items_.empty()) { if (!this->items_.empty()) {
auto &last_item = this->items_.back(); auto &last_item = this->items_.back();
if (this->matches_item_(last_item, component, name_cstr, type, match_retry)) { if (this->matches_item_locked_(last_item, component, name_cstr, type, match_retry)) {
this->recycle_item_(std::move(this->items_.back())); this->recycle_item_(std::move(this->items_.back()));
this->items_.pop_back(); this->items_.pop_back();
total_cancelled++; total_cancelled++;
} }
// For other items in heap, we can only mark for removal (can't remove from middle of heap) // For other items in heap, we can only mark for removal (can't remove from middle of heap)
size_t heap_cancelled = this->mark_matching_items_removed_(this->items_, component, name_cstr, type, match_retry); size_t heap_cancelled =
this->mark_matching_items_removed_locked_(this->items_, component, name_cstr, type, match_retry);
total_cancelled += heap_cancelled; total_cancelled += heap_cancelled;
this->to_remove_ += heap_cancelled; // Track removals for heap items this->to_remove_ += heap_cancelled; // Track removals for heap items
} }
// Cancel items in to_add_ // Cancel items in to_add_
total_cancelled += this->mark_matching_items_removed_(this->to_add_, component, name_cstr, type, match_retry); total_cancelled += this->mark_matching_items_removed_locked_(this->to_add_, component, name_cstr, type, match_retry);
return total_cancelled > 0; return total_cancelled > 0;
} }

35
esphome/core/scheduler.h
View File

@@ -243,8 +243,18 @@ class Scheduler {
} }
// Helper function to check if item matches criteria for cancellation // Helper function to check if item matches criteria for cancellation
inline bool HOT matches_item_(const std::unique_ptr<SchedulerItem> &item, Component *component, const char *name_cstr, // IMPORTANT: Must be called with scheduler lock held
SchedulerItem::Type type, bool match_retry, bool skip_removed = true) const { inline bool HOT matches_item_locked_(const std::unique_ptr<SchedulerItem> &item, Component *component,
const char *name_cstr, SchedulerItem::Type type, bool match_retry,
bool skip_removed = true) const {
// THREAD SAFETY: Check for nullptr first to prevent LoadProhibited crashes. On multi-threaded
// platforms, items can be moved out of defer_queue_ during processing, leaving nullptr entries.
// PR #11305 added nullptr checks in callers (mark_matching_items_removed_locked_() and
// has_cancelled_timeout_in_container_locked_()), but this check provides defense-in-depth: helper
// functions should be safe regardless of caller behavior.
// Fixes: https://github.com/esphome/esphome/issues/11940
if (!item)
return false;
if (item->component != component || item->type != type || (skip_removed && item->remove) || if (item->component != component || item->type != type || (skip_removed && item->remove) ||
(match_retry && !item->is_retry)) { (match_retry && !item->is_retry)) {
return false; return false;
@@ -304,8 +314,8 @@ class Scheduler {
// SAFETY: Moving out the unique_ptr leaves a nullptr in the vector at defer_queue_front_. // SAFETY: Moving out the unique_ptr leaves a nullptr in the vector at defer_queue_front_.
// This is intentional and safe because: // This is intentional and safe because:
// 1. The vector is only cleaned up by cleanup_defer_queue_locked_() at the end of this function // 1. The vector is only cleaned up by cleanup_defer_queue_locked_() at the end of this function
// 2. Any code iterating defer_queue_ MUST check for nullptr items (see mark_matching_items_removed_ // 2. Any code iterating defer_queue_ MUST check for nullptr items (see mark_matching_items_removed_locked_
// and has_cancelled_timeout_in_container_ in scheduler.h) // and has_cancelled_timeout_in_container_locked_ in scheduler.h)
// 3. The lock protects concurrent access, but the nullptr remains until cleanup // 3. The lock protects concurrent access, but the nullptr remains until cleanup
item = std::move(this->defer_queue_[this->defer_queue_front_]); item = std::move(this->defer_queue_[this->defer_queue_front_]);
this->defer_queue_front_++; this->defer_queue_front_++;
@@ -393,10 +403,10 @@ class Scheduler {
// Helper to mark matching items in a container as removed // Helper to mark matching items in a container as removed
// Returns the number of items marked for removal // Returns the number of items marked for removal
// IMPORTANT: Caller must hold the scheduler lock before calling this function. // IMPORTANT: Must be called with scheduler lock held
template<typename Container> template<typename Container>
size_t mark_matching_items_removed_(Container &container, Component *component, const char *name_cstr, size_t mark_matching_items_removed_locked_(Container &container, Component *component, const char *name_cstr,
SchedulerItem::Type type, bool match_retry) { SchedulerItem::Type type, bool match_retry) {
size_t count = 0; size_t count = 0;
for (auto &item : container) { for (auto &item : container) {
// Skip nullptr items (can happen in defer_queue_ when items are being processed) // Skip nullptr items (can happen in defer_queue_ when items are being processed)
@@ -405,7 +415,7 @@ class Scheduler {
// the vector can still contain nullptr items from the processing loop. This check prevents crashes. // the vector can still contain nullptr items from the processing loop. This check prevents crashes.
if (!item) if (!item)
continue; continue;
if (this->matches_item_(item, component, name_cstr, type, match_retry)) { if (this->matches_item_locked_(item, component, name_cstr, type, match_retry)) {
// Mark item for removal (platform-specific) // Mark item for removal (platform-specific)
this->set_item_removed_(item.get(), true); this->set_item_removed_(item.get(), true);
count++; count++;
@@ -415,9 +425,10 @@ class Scheduler {
} }
// Template helper to check if any item in a container matches our criteria // Template helper to check if any item in a container matches our criteria
// IMPORTANT: Must be called with scheduler lock held
template<typename Container> template<typename Container>
bool has_cancelled_timeout_in_container_(const Container &container, Component *component, const char *name_cstr, bool has_cancelled_timeout_in_container_locked_(const Container &container, Component *component,
bool match_retry) const { const char *name_cstr, bool match_retry) const {
for (const auto &item : container) { for (const auto &item : container) {
// Skip nullptr items (can happen in defer_queue_ when items are being processed) // Skip nullptr items (can happen in defer_queue_ when items are being processed)
// The defer_queue_ uses index-based processing: items are std::moved out but left in the // The defer_queue_ uses index-based processing: items are std::moved out but left in the
@@ -426,8 +437,8 @@ class Scheduler {
if (!item) if (!item)
continue; continue;
if (is_item_removed_(item.get()) && if (is_item_removed_(item.get()) &&
this->matches_item_(item, component, name_cstr, SchedulerItem::TIMEOUT, match_retry, this->matches_item_locked_(item, component, name_cstr, SchedulerItem::TIMEOUT, match_retry,
/* skip_removed= */ false)) { /* skip_removed= */ false)) {
return true; return true;
} }
} }