kopia lustrzana https://github.com/espressif/esp-idf
improve thread safety in esp_timer
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.pull/11519/head
rodzic
b97cf6ca7c
commit
07d1e19107
|
@ -200,18 +200,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;
|
||||
}
|
||||
|
@ -221,20 +233,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;
|
||||
}
|
||||
|
@ -244,10 +263,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)
|
||||
|
@ -255,22 +286,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)
|
||||
|
|
Ładowanie…
Reference in New Issue