From 31b7db5c38b39907b541a5dd6ded7dad1f94bfd0 Mon Sep 17 00:00:00 2001
From: Ivan Grokhotkov <ivan@espressif.com>
Date: Wed, 19 Dec 2018 15:53:50 +0800
Subject: [PATCH] esp_timer: do not allow deleting timers while callbacks are
 dispatched

timer_process_alarm function of esp_timer holds a spinlock for the
entire duration of its operation, except for the time when timer
callback function is called. It is possible that when
timer_process_alarm releases the spinlock, a higher priority task may
run and delete the timer. Then the execution will return to
timer_process_alarm, and this will either cause a crash, or undesired
execution of callback after the timer has been stopped or deleted.

To solve this problem, add a mutex which will prevent deletion of timers
while callbacks are being dispatched.
---
 components/esp32/esp_timer.c | 59 +++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/components/esp32/esp_timer.c b/components/esp32/esp_timer.c
index ded2de8de..78f004acf 100644
--- a/components/esp32/esp_timer.c
+++ b/components/esp32/esp_timer.c
@@ -83,10 +83,13 @@ static esp_timer_handle_t s_timer_in_callback;
 static TaskHandle_t s_timer_task;
 // counting semaphore used to notify the timer task from ISR
 static SemaphoreHandle_t s_timer_semaphore;
+// mutex which protects timers from deletion during callback execution
+static SemaphoreHandle_t s_timer_delete_mutex;
 
 #if CONFIG_SPIRAM_USE_MALLOC
-// memory for s_timer_semaphore
+// memory for s_timer_semaphore and s_timer_delete_mutex
 static StaticQueue_t s_timer_semaphore_memory;
+static StaticQueue_t s_timer_delete_mutex_memory;
 #endif
 
 // lock protecting s_timers, s_inactive_timers, s_timer_in_callback
@@ -154,19 +157,21 @@ esp_err_t IRAM_ATTR esp_timer_stop(esp_timer_handle_t timer)
 
 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;
     }
+    xSemaphoreTakeRecursive(s_timer_delete_mutex, portMAX_DELAY);
 #if WITH_PROFILING
     if (timer == s_timer_in_callback) {
         s_timer_in_callback = NULL;
     }
     timer_remove_inactive(timer);
 #endif
-    if (timer == NULL) {
-        return ESP_ERR_INVALID_ARG;
-    }
     free(timer);
+    xSemaphoreGiveRecursive(s_timer_delete_mutex);
     return ESP_OK;
 }
 
@@ -261,6 +266,7 @@ static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
     /* unused, provision to allow running callbacks from ISR */
     (void) dispatch_method;
 
+    xSemaphoreTakeRecursive(s_timer_delete_mutex, portMAX_DELAY);
     timer_list_lock();
     uint64_t now = esp_timer_impl_get_time();
     esp_timer_handle_t it = LIST_FIRST(&s_timers);
@@ -301,6 +307,7 @@ static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
         esp_timer_impl_set_alarm(first->alarm);
     }
     timer_list_unlock();
+    xSemaphoreGiveRecursive(s_timer_delete_mutex);
 }
 
 static void timer_task(void* arg)
@@ -332,6 +339,7 @@ static IRAM_ATTR bool is_initialized()
 
 esp_err_t esp_timer_init(void)
 {
+    esp_err_t err;
     if (is_initialized()) {
         return ESP_ERR_INVALID_STATE;
     }
@@ -343,27 +351,50 @@ esp_err_t esp_timer_init(void)
     s_timer_semaphore = xSemaphoreCreateCounting(TIMER_EVENT_QUEUE_SIZE, 0);
 #endif
     if (!s_timer_semaphore) {
-        return ESP_ERR_NO_MEM;
+        err = ESP_ERR_NO_MEM;
+        goto out;
     }
 
+#if CONFIG_SPIRAM_USE_MALLOC
+    memset(&s_timer_delete_mutex_memory, 0, sizeof(StaticQueue_t));
+    s_timer_delete_mutex = xSemaphoreCreateRecursiveMutexStatic(&s_timer_delete_mutex_memory);
+#else
+    s_timer_delete_mutex = xSemaphoreCreateRecursiveMutex();
+#endif
+    if (!s_timer_delete_mutex) {
+        err = ESP_ERR_NO_MEM;
+        goto out;
+    }
+
+
     int ret = xTaskCreatePinnedToCore(&timer_task, "esp_timer",
             ESP_TASK_TIMER_STACK, NULL, ESP_TASK_TIMER_PRIO, &s_timer_task, PRO_CPU_NUM);
     if (ret != pdPASS) {
-        vSemaphoreDelete(s_timer_semaphore);
-        s_timer_semaphore = NULL;
-        return ESP_ERR_NO_MEM;
+        err = ESP_ERR_NO_MEM;
+        goto out;
     }
 
-    esp_err_t err = esp_timer_impl_init(&timer_alarm_handler);
+    err = esp_timer_impl_init(&timer_alarm_handler);
     if (err != ESP_OK) {
-        vTaskDelete(s_timer_task);
-        s_timer_task = NULL;
-        vSemaphoreDelete(s_timer_semaphore);
-        s_timer_semaphore = NULL;
-        return err;
+        goto out;
     }
 
     return ESP_OK;
+
+out:
+    if (s_timer_task) {
+        vTaskDelete(s_timer_task);
+        s_timer_task = NULL;
+    }
+    if (s_timer_semaphore) {
+        vSemaphoreDelete(s_timer_semaphore);
+        s_timer_semaphore = NULL;
+    }
+    if (s_timer_delete_mutex) {
+        vSemaphoreDelete(s_timer_delete_mutex);
+        s_timer_delete_mutex = NULL;
+    }
+    return ESP_ERR_NO_MEM;
 }
 
 esp_err_t esp_timer_deinit(void)