alex/esp-idf
alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2024-10-05 20:47:46 -04:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

improve thread safety in esp_timer

Browse Source 
Inadequate locking in the esp_timer component allowed corruption
of the s_timers linked list:

1. timer_armed(timer) returns false
2. another task arms the timer and adds it to s_timers
3. the list is locked
4. the timer is inserted into s_timers again

The last step results in a loop in the s_timers list, which causes
an infinite loop when iterated. This change always locks the
list before checking if the timer is already armed avoiding
the data race.
This commit is contained in:
Jens Gutermuth 2022-03-22 13:59:33 +01:00
parent f93bb3a63c
commit 3ba70490c9
1 changed files with 62 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

88
components/esp_timer/src/esp_timer.c
View File

@ -196,18 +196,30 @@ esp_err_t IRAM_ATTR esp_timer_start_once(esp_timer_handle_t timer, uint64_t time
if (timer == NULL) {
return ESP_ERR_INVALID_ARG;
}
if (!is_initialized() || timer_armed(timer)) {
if (!is_initialized()) {
return ESP_ERR_INVALID_STATE;
}
int64_t alarm = esp_timer_get_time() + timeout_us;
esp_timer_dispatch_t dispatch_method = timer->flags & FL_ISR_DISPATCH_METHOD;
esp_err_t err;
timer_list_lock(dispatch_method);
timer->alarm = alarm;
timer->period = 0;
/* Check if the timer is armed once the list is locked.
* Otherwise another task may arm the timer inbetween the check
* and us locking the list, resulting in us inserting the
* timer to s_timers a second time. This will create a loop
* in s_timers. */
if (timer_armed(timer)) {
err = ESP_ERR_INVALID_STATE;
} else {
timer->alarm = alarm;
timer->period = 0;
#if WITH_PROFILING
timer->times_armed++;
timer->times_armed++;
#endif
esp_err_t err = timer_insert(timer, false);
err = timer_insert(timer, false);
}
timer_list_unlock(dispatch_method);
return err;
}
@ -217,20 +229,27 @@ esp_err_t IRAM_ATTR esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t
if (timer == NULL) {
return ESP_ERR_INVALID_ARG;
}
if (!is_initialized() || timer_armed(timer)) {
if (!is_initialized()) {
return ESP_ERR_INVALID_STATE;
}
period_us = MAX(period_us, esp_timer_impl_get_min_period_us());
int64_t alarm = esp_timer_get_time() + period_us;
esp_timer_dispatch_t dispatch_method = timer->flags & FL_ISR_DISPATCH_METHOD;
esp_err_t err;
timer_list_lock(dispatch_method);
timer->alarm = alarm;
timer->period = period_us;
/* Check if the timer is armed once the list is locked to avoid a data race */
if (timer_armed(timer)) {
err = ESP_ERR_INVALID_STATE;
} else {
timer->alarm = alarm;
timer->period = period_us;
#if WITH_PROFILING
timer->times_armed++;
timer->times_skipped = 0;
timer->times_armed++;
timer->times_skipped = 0;
#endif
esp_err_t err = timer_insert(timer, false);
err = timer_insert(timer, false);
}
timer_list_unlock(dispatch_method);
return err;
}
@ -240,10 +259,22 @@ esp_err_t IRAM_ATTR esp_timer_stop(esp_timer_handle_t timer)
if (timer == NULL) {
return ESP_ERR_INVALID_ARG;
}
if (!is_initialized() || !timer_armed(timer)) {
if (!is_initialized()) {
return ESP_ERR_INVALID_STATE;
}
return timer_remove(timer);
esp_timer_dispatch_t dispatch_method = timer->flags & FL_ISR_DISPATCH_METHOD;
esp_err_t err;
timer_list_lock(dispatch_method);
/* Check if the timer is armed once the list is locked to avoid a data race */
if (!timer_armed(timer)) {
err = ESP_ERR_INVALID_STATE;
} else {
err = timer_remove(timer);
}
timer_list_unlock(dispatch_method);
return err;
}
esp_err_t esp_timer_delete(esp_timer_handle_t timer)
@ -251,22 +282,27 @@ esp_err_t esp_timer_delete(esp_timer_handle_t timer)
if (timer == NULL) {
return ESP_ERR_INVALID_ARG;
}
if (timer_armed(timer)) {
return ESP_ERR_INVALID_STATE;
}
// A case for the timer with ESP_TIMER_ISR:
// This ISR timer was removed from the ISR list in esp_timer_stop() or in timer_process_alarm() -> LIST_REMOVE(it, list_entry)
// and here this timer will be added to another the TASK list, see below.
// We do this because we want to free memory of the timer in a task context instead of an isr context.
int64_t alarm = esp_timer_get_time();
esp_err_t err;
timer_list_lock(ESP_TIMER_TASK);
timer->flags &= ~FL_ISR_DISPATCH_METHOD;
timer->event_id = EVENT_ID_DELETE_TIMER;
timer->alarm = alarm;
timer->period = 0;
timer_insert(timer, false);
/* Check if the timer is armed once the list is locked to avoid a data race */
if (timer_armed(timer)) {
err = ESP_ERR_INVALID_STATE;
} else {
// A case for the timer with ESP_TIMER_ISR:
// This ISR timer was removed from the ISR list in esp_timer_stop() or in timer_process_alarm() -> LIST_REMOVE(it, list_entry)
// and here this timer will be added to another the TASK list, see below.
// We do this because we want to free memory of the timer in a task context instead of an isr context.
timer->flags &= ~FL_ISR_DISPATCH_METHOD;
timer->event_id = EVENT_ID_DELETE_TIMER;
timer->alarm = alarm;
timer->period = 0;
err = timer_insert(timer, false);
}
timer_list_unlock(ESP_TIMER_TASK);
return ESP_OK;
return err;
}
static IRAM_ATTR esp_err_t timer_insert(esp_timer_handle_t timer, bool without_update_alarm)