mirror of
https://github.com/esphome/esphome.git
synced 2025-07-28 14:16:40 +00:00
Fix scheduler crash when cancelling items with NULL names (#9444)
This commit is contained in:
parent
475fe60f27
commit
bef20b60d0
@ -66,10 +66,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type
|
|||||||
|
|
||||||
if (delay == SCHEDULER_DONT_RUN) {
|
if (delay == SCHEDULER_DONT_RUN) {
|
||||||
// Still need to cancel existing timer if name is not empty
|
// Still need to cancel existing timer if name is not empty
|
||||||
if (this->is_name_valid_(name_cstr)) {
|
LockGuard guard{this->lock_};
|
||||||
LockGuard guard{this->lock_};
|
this->cancel_item_locked_(component, name_cstr, type);
|
||||||
this->cancel_item_locked_(component, name_cstr, type);
|
|
||||||
}
|
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -125,10 +123,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type
|
|||||||
|
|
||||||
LockGuard guard{this->lock_};
|
LockGuard guard{this->lock_};
|
||||||
// If name is provided, do atomic cancel-and-add
|
// If name is provided, do atomic cancel-and-add
|
||||||
if (this->is_name_valid_(name_cstr)) {
|
// Cancel existing items
|
||||||
// Cancel existing items
|
this->cancel_item_locked_(component, name_cstr, type);
|
||||||
this->cancel_item_locked_(component, name_cstr, type);
|
|
||||||
}
|
|
||||||
// Add new item directly to to_add_
|
// Add new item directly to to_add_
|
||||||
// since we have the lock held
|
// since we have the lock held
|
||||||
this->to_add_.push_back(std::move(item));
|
this->to_add_.push_back(std::move(item));
|
||||||
@ -442,10 +438,6 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co
|
|||||||
// Get the name as const char*
|
// Get the name as const char*
|
||||||
const char *name_cstr = this->get_name_cstr_(is_static_string, name_ptr);
|
const char *name_cstr = this->get_name_cstr_(is_static_string, name_ptr);
|
||||||
|
|
||||||
// Handle null or empty names
|
|
||||||
if (!this->is_name_valid_(name_cstr))
|
|
||||||
return false;
|
|
||||||
|
|
||||||
// obtain lock because this function iterates and can be called from non-loop task context
|
// obtain lock because this function iterates and can be called from non-loop task context
|
||||||
LockGuard guard{this->lock_};
|
LockGuard guard{this->lock_};
|
||||||
return this->cancel_item_locked_(component, name_cstr, type);
|
return this->cancel_item_locked_(component, name_cstr, type);
|
||||||
@ -453,6 +445,11 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co
|
|||||||
|
|
||||||
// Helper to cancel items by name - must be called with lock held
|
// Helper to cancel items by name - must be called with lock held
|
||||||
bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) {
|
bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) {
|
||||||
|
// Early return if name is invalid - no items to cancel
|
||||||
|
if (name_cstr == nullptr || name_cstr[0] == '\0') {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
size_t total_cancelled = 0;
|
size_t total_cancelled = 0;
|
||||||
|
|
||||||
// Check all containers for matching items
|
// Check all containers for matching items
|
||||||
|
@ -150,9 +150,6 @@ class Scheduler {
|
|||||||
return is_static_string ? static_cast<const char *>(name_ptr) : static_cast<const std::string *>(name_ptr)->c_str();
|
return is_static_string ? static_cast<const char *>(name_ptr) : static_cast<const std::string *>(name_ptr)->c_str();
|
||||||
}
|
}
|
||||||
|
|
||||||
// Helper to check if a name is valid (not null and not empty)
|
|
||||||
inline bool is_name_valid_(const char *name) { return name != nullptr && name[0] != '\0'; }
|
|
||||||
|
|
||||||
// Common implementation for cancel operations
|
// Common implementation for cancel operations
|
||||||
bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type);
|
bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type);
|
||||||
|
|
||||||
|
43
tests/integration/fixtures/scheduler_null_name.yaml
Normal file
43
tests/integration/fixtures/scheduler_null_name.yaml
Normal file
@ -0,0 +1,43 @@
|
|||||||
|
esphome:
|
||||||
|
name: scheduler-null-name
|
||||||
|
|
||||||
|
host:
|
||||||
|
|
||||||
|
logger:
|
||||||
|
level: DEBUG
|
||||||
|
|
||||||
|
api:
|
||||||
|
services:
|
||||||
|
- service: test_null_name
|
||||||
|
then:
|
||||||
|
- lambda: |-
|
||||||
|
// First, create a scenario that would trigger the crash
|
||||||
|
// The crash happens when defer() is called with a name that would be cancelled
|
||||||
|
|
||||||
|
// Test 1: Create a defer with a valid name
|
||||||
|
App.scheduler.set_timeout(nullptr, "test_defer", 0, []() {
|
||||||
|
ESP_LOGI("TEST", "First defer should be cancelled");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Test 2: Create another defer with the same name - this triggers cancel_item_locked_
|
||||||
|
// In the unfixed code, this would crash if the name was NULL
|
||||||
|
App.scheduler.set_timeout(nullptr, "test_defer", 0, []() {
|
||||||
|
ESP_LOGI("TEST", "Second defer executed");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Test 3: Now test with nullptr - this is the actual crash scenario
|
||||||
|
// Create a defer item without a name (like voice assistant does)
|
||||||
|
const char* null_name = nullptr;
|
||||||
|
App.scheduler.set_timeout(nullptr, null_name, 0, []() {
|
||||||
|
ESP_LOGI("TEST", "Defer with null name executed");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Test 4: Create another defer with null name - this would trigger the crash
|
||||||
|
App.scheduler.set_timeout(nullptr, null_name, 0, []() {
|
||||||
|
ESP_LOGI("TEST", "Second null defer executed");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Test 5: Verify scheduler still works
|
||||||
|
App.scheduler.set_timeout(nullptr, "valid_timeout", 50, []() {
|
||||||
|
ESP_LOGI("TEST", "Test completed successfully");
|
||||||
|
});
|
59
tests/integration/test_scheduler_null_name.py
Normal file
59
tests/integration/test_scheduler_null_name.py
Normal file
@ -0,0 +1,59 @@
|
|||||||
|
"""Test that scheduler handles NULL names safely without crashing."""
|
||||||
|
|
||||||
|
import asyncio
|
||||||
|
import re
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from .types import APIClientConnectedFactory, RunCompiledFunction
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_scheduler_null_name(
|
||||||
|
yaml_config: str,
|
||||||
|
run_compiled: RunCompiledFunction,
|
||||||
|
api_client_connected: APIClientConnectedFactory,
|
||||||
|
) -> None:
|
||||||
|
"""Test that scheduler handles NULL names safely without crashing."""
|
||||||
|
|
||||||
|
loop = asyncio.get_running_loop()
|
||||||
|
test_complete_future: asyncio.Future[bool] = loop.create_future()
|
||||||
|
|
||||||
|
# Pattern to match test completion
|
||||||
|
test_complete_pattern = re.compile(r"Test completed successfully")
|
||||||
|
|
||||||
|
def check_output(line: str) -> None:
|
||||||
|
"""Check log output for test completion."""
|
||||||
|
if not test_complete_future.done() and test_complete_pattern.search(line):
|
||||||
|
test_complete_future.set_result(True)
|
||||||
|
|
||||||
|
async with run_compiled(yaml_config, line_callback=check_output):
|
||||||
|
async with api_client_connected() as client:
|
||||||
|
# Verify we can connect
|
||||||
|
device_info = await client.device_info()
|
||||||
|
assert device_info is not None
|
||||||
|
assert device_info.name == "scheduler-null-name"
|
||||||
|
|
||||||
|
# List services
|
||||||
|
_, services = await asyncio.wait_for(
|
||||||
|
client.list_entities_services(), timeout=5.0
|
||||||
|
)
|
||||||
|
|
||||||
|
# Find our test service
|
||||||
|
test_null_name_service = next(
|
||||||
|
(s for s in services if s.name == "test_null_name"), None
|
||||||
|
)
|
||||||
|
assert test_null_name_service is not None, (
|
||||||
|
"test_null_name service not found"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Execute the test
|
||||||
|
client.execute_service(test_null_name_service, {})
|
||||||
|
|
||||||
|
# Wait for test completion
|
||||||
|
try:
|
||||||
|
await asyncio.wait_for(test_complete_future, timeout=10.0)
|
||||||
|
except asyncio.TimeoutError:
|
||||||
|
pytest.fail(
|
||||||
|
"Test did not complete within timeout - likely crashed due to NULL name"
|
||||||
|
)
|
Loading…
x
Reference in New Issue
Block a user