From 457b71f1a36c876367b95e10faef479a5306eb2c Mon Sep 17 00:00:00 2001
From: Jens Gutermuth <jens.gutermuth@inro-et.de>
Date: Tue, 22 Mar 2022 13:59:33 +0100
Subject: [PATCH] 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.
---
 components/esp_timer/src/esp_timer.c | 72 ++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 20 deletions(-)

diff --git a/components/esp_timer/src/esp_timer.c b/components/esp_timer/src/esp_timer.c
index 2bbb439e5e..23ca75883f 100644
--- a/components/esp_timer/src/esp_timer.c
+++ b/components/esp_timer/src/esp_timer.c
@@ -126,16 +126,26 @@ 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;
     }
     timer_list_lock();
-    timer->alarm = esp_timer_get_time() + timeout_us;
-    timer->period = 0;
+    esp_err_t err;
+    /* 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 = esp_timer_get_time() + timeout_us;
+        timer->period = 0;
 #if WITH_PROFILING
-    timer->times_armed++;
+        timer->times_armed++;
 #endif
-    esp_err_t err = timer_insert(timer);
+        err = timer_insert(timer);
+    }
     timer_list_unlock();
     return err;
 }
@@ -145,17 +155,24 @@ 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;
     }
     timer_list_lock();
     period_us = MAX(period_us, esp_timer_impl_get_min_period_us());
-    timer->alarm = esp_timer_get_time() + period_us;
-    timer->period = period_us;
+    esp_err_t err;
+
+    /* 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 = esp_timer_get_time() + period_us;
+        timer->period = period_us;
 #if WITH_PROFILING
-    timer->times_armed++;
+        timer->times_armed++;
 #endif
-    esp_err_t err = timer_insert(timer);
+        err = timer_insert(timer);
+    }
     timer_list_unlock();
     return err;
 }
@@ -165,10 +182,21 @@ 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_err_t err;
+
+    timer_list_lock();
+
+    /* 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();
+    return err;
 }
 
 esp_err_t esp_timer_delete(esp_timer_handle_t timer)
@@ -176,16 +204,20 @@ 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;
-    }
+    esp_err_t err;
     timer_list_lock();
-    timer->event_id = EVENT_ID_DELETE_TIMER;
-    timer->alarm = esp_timer_get_time();
-    timer->period = 0;
-    timer_insert(timer);
+
+    /* 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->event_id = EVENT_ID_DELETE_TIMER;
+        timer->alarm = esp_timer_get_time();
+        timer->period = 0;
+        err = timer_insert(timer);
+    }
     timer_list_unlock();
-    return ESP_OK;
+    return err;
 }
 
 static IRAM_ATTR esp_err_t timer_insert(esp_timer_handle_t timer)