Merge branch 'fix/vtaskdeletewithcaps_leaks_memory' into 'master'

fix(freertos): Fixed memory leak issue in vTaskDeleteWithCaps()

Closes IDFGH-13294

See merge request espressif/esp-idf!32305

components/freertos/FreeRTOS-Kernel-SMP/portable/linux/include/freertos/portmacro.h

@@ -244,6 +244,12 @@ static inline BaseType_t xPortInIsrContext(void)
     return xPortCheckIfInISR();
 }
 
+static inline void vPortAssertIfInISR(void)
+{
+    /* Assert if the interrupt nesting count is > 0 */
+    configASSERT(xPortInIsrContext() == 0);
+}
+
 // xPortInterruptedFromISRContext() is only used in panic handler and core dump,
 // both probably not relevant on POSIX sim.
 //BaseType_t xPortInterruptedFromISRContext(void);
@@ -301,7 +307,7 @@ extern void vPortCancelThread( void *pxTaskToDelete );
  * are always a full memory barrier. ISRs are emulated as signals
  * which also imply a full memory barrier.
  *
- * Thus, only a compilier barrier is needed to prevent the compiler
+ * Thus, only a compiler barrier is needed to prevent the compiler
  * reordering.
  */
 #define portMEMORY_BARRIER() __asm volatile( "" ::: "memory" )

components/freertos/FreeRTOS-Kernel/portable/linux/include/freertos/portmacro_idf.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -104,6 +104,17 @@ static inline BaseType_t xPortInIsrContext(void)
     return xPortCheckIfInISR();
 }
 
+static inline void vPortAssertIfInISR(void)
+{
+    /* Assert if the interrupt nesting count is > 0 */
+    configASSERT(xPortInIsrContext() == 0);
+}
+
+/**
+ * @brief Assert if in ISR context
+ */
+#define portASSERT_IF_IN_ISR() vPortAssertIfInISR()
+
 #if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP
 /* If enabled, users must provide an implementation of vPortCleanUpTCB() */
 extern void vPortCleanUpTCB ( void *pxTCB );

components/freertos/esp_additions/idf_additions.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -21,6 +21,8 @@
 #include "freertos/timers.h"
 #include "freertos/idf_additions.h"
 #include "esp_heap_caps.h"
+#include "esp_log.h"
+#include "freertos/portmacro.h"
 
 /* -------------------------------------------- Creation With Memory Caps ------------------------------------------- */
 
@@ -81,21 +83,128 @@ err:
 
 #if ( configSUPPORT_STATIC_ALLOCATION == 1 )
 
+    static void prvTaskDeleteWithCapsTask( void * pvParameters )
+    {
+        TaskHandle_t xTaskToDelete = ( TaskHandle_t ) pvParameters;
+
+        /* The task to be deleted must not be running */
+        configASSERT( eRunning != eTaskGetState( xTaskToDelete ) );
+
+        /* Delete the WithCaps task */
+        vTaskDeleteWithCaps( xTaskToDelete );
+
+        /* Delete the temporary clean up task */
+        vTaskDelete( NULL );
+    }
+
     void vTaskDeleteWithCaps( TaskHandle_t xTaskToDelete )
     {
-        BaseType_t xResult;
-        StaticTask_t * pxTaskBuffer;
-        StackType_t * puxStackBuffer;
+        /* THIS FUNCTION SHOULD NOT BE CALLED FROM AN INTERRUPT CONTEXT. */
+        /*TODO: Update it to use portASSERT_IF_IN_ISR() instead. (IDF-10540) */
+        vPortAssertIfInISR();
 
-        xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer );
-        configASSERT( xResult == pdTRUE );
+        TaskHandle_t xCurrentTaskHandle = xTaskGetCurrentTaskHandle();
+        configASSERT( xCurrentTaskHandle != NULL );
 
-        /* Delete the task */
-        vTaskDelete( xTaskToDelete );
+        if( ( xTaskToDelete == NULL ) || ( xTaskToDelete == xCurrentTaskHandle ) )
+        {
+            /* The WithCaps task is deleting itself. While, the task can put itself on the
+             * xTasksWaitingTermination list via the vTaskDelete() call, the idle
+             * task will not free the task TCB and stack memories we created statically
+             * during xTaskCreateWithCaps() or xTaskCreatePinnedToCoreWithCaps(). This
+             * task will never be rescheduled once it is on the xTasksWaitingTermination
+             * list and will not be able to clear the memories. Therefore, it will leak memory.
+             *
+             * To avoid this, we create a new "temporary clean up" task to delete the current task.
+             * This task is created at the priority of the task to be deleted with the same core
+             * affitinty. Its limited purpose is to delete the self-deleting task created WithCaps.
+             *
+             * This approach has the following problems -
+             * 1. Once a WithCaps task deletes itself via vTaskDeleteWithCaps(), it may end up in the
+             *    suspended tasks lists for a short time before being deleted. This can give an incorrect
+             *    picture about the system state.
+             *
+             * 2. This approach is wasteful and can be error prone. The temporary clean up task will need
+             *    system resources to get scheduled and cleanup the WithCaps task. It can be a problem if the system
+             *    has several self-deleting WithCaps tasks.
+             *
+             * TODO: A better approach could be either -
+             *
+             * 1. Delegate memory management to the application/user. This way the kernel needn't bother about freeing
+             *    the memory (like other static memory task creation APIs like xTaskCreateStatic()) (IDF-10521)
+             *
+             * 2. Have a post deletion hook/callback from the IDLE task to notify higher layers when it is safe to
+             *    perform activities such as clearing up the TCB and stack memories. (IDF-10522) */
+            if( xTaskCreatePinnedToCore( ( TaskFunction_t ) prvTaskDeleteWithCapsTask, "prvTaskDeleteWithCapsTask", configMINIMAL_STACK_SIZE, xCurrentTaskHandle, uxTaskPriorityGet( xTaskToDelete ), NULL, xPortGetCoreID() ) != pdFAIL )
+            {
+                /* Although the current task should get preemted immediately when prvTaskDeleteWithCapsTask is created,
+                 * for safety, we suspend the current task and wait for prvTaskDeleteWithCapsTask to delete it. */
+                vTaskSuspend( xTaskToDelete );
 
-        /* Free the memory buffers */
-        heap_caps_free( puxStackBuffer );
-        vPortFree( pxTaskBuffer );
+                /* Should never reach here */
+                ESP_LOGE( "freertos_additions", "%s: Failed to suspend the task to be deleted", __func__ );
+                abort();
+            }
+            else
+            {
+                /* Failed to create the task to delete the current task. */
+                ESP_LOGE( "freertos_additions", "%s: Failed to create the task to delete the current task", __func__ );
+                abort();
+            }
+        }
+
+        #if ( configNUM_CORES > 1 )
+            else if( eRunning == eTaskGetState( xTaskToDelete ) )
+            {
+                /* The WithCaps task is running on another core.
+                 * We suspend the task first and then delete it. */
+                vTaskSuspend( xTaskToDelete );
+
+                /* Wait for the task to be suspended */
+                while( eRunning == eTaskGetState( xTaskToDelete ) )
+                {
+                    portYIELD_WITHIN_API();
+                }
+
+                BaseType_t xResult;
+                StaticTask_t * pxTaskBuffer;
+                StackType_t * puxStackBuffer;
+
+                xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer );
+                configASSERT( xResult == pdTRUE );
+                configASSERT( puxStackBuffer != NULL );
+                configASSERT( pxTaskBuffer != NULL );
+
+                /* Delete the task */
+                vTaskDelete( xTaskToDelete );
+
+                /* Free the memory buffers */
+                heap_caps_free( puxStackBuffer );
+                vPortFree( pxTaskBuffer );
+            }
+        #endif /* if ( configNUM_CORES > 1 ) */
+        else
+        {
+            /* The WithCaps task is not running and is being deleted
+             * from another task's context. */
+            configASSERT( eRunning != eTaskGetState( xTaskToDelete ) );
+
+            BaseType_t xResult;
+            StaticTask_t * pxTaskBuffer;
+            StackType_t * puxStackBuffer;
+
+            xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer );
+            configASSERT( xResult == pdTRUE );
+            configASSERT( puxStackBuffer != NULL );
+            configASSERT( pxTaskBuffer != NULL );
+
+            /* We can delete the task and free the memory buffers. */
+            vTaskDelete( xTaskToDelete );
+
+            /* Free the memory buffers */
+            heap_caps_free( puxStackBuffer );
+            vPortFree( pxTaskBuffer );
+        } /* if( ( xTaskToDelete == NULL ) || ( xTaskToDelete == xCurrentTaskHandle ) ) */
     }
 
 #endif /* if ( configSUPPORT_STATIC_ALLOCATION == 1 ) */

components/freertos/esp_additions/include/freertos/idf_additions.h

@@ -339,6 +339,11 @@ uint8_t * pxTaskGetStackStart( TaskHandle_t xTask );
  * @brief Deletes a task previously created using xTaskCreateWithCaps() or
  * xTaskCreatePinnedToCoreWithCaps()
  *
+ * @note It is recommended to use this API to delete tasks from another task's
+ * context, rather than self-deletion. When the task is being deleted, it is vital
+ * to ensure that it is not running on another core. This API must not be called
+ * from an interrupt context.
+ *
  * @param xTaskToDelete A handle to the task to be deleted
  */
     void vTaskDeleteWithCaps( TaskHandle_t xTaskToDelete );

components/freertos/test_apps/freertos/misc/test_idf_additions.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -43,7 +43,17 @@ static void task_with_caps(void *arg)
     vTaskSuspend(NULL);
 }
 
-TEST_CASE("IDF additions: Task creation with memory caps", "[freertos]")
+static void task_with_caps_self_delete(void *arg)
+{
+    /* Wait for the unity task to indicate that this task should delete itself */
+    ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+
+    /* Although it is not recommended to self-delete a task with memory caps but this
+     * is done intentionally to test for memory leaks */
+    vTaskDeleteWithCaps(NULL);
+}
+
+TEST_CASE("IDF additions: Task creation with memory caps and deletion from another task", "[freertos]")
 {
     TaskHandle_t task_handle = NULL;
     StackType_t *puxStackBuffer;
@@ -62,6 +72,57 @@ TEST_CASE("IDF additions: Task creation with memory caps", "[freertos]")
     vTaskDeleteWithCaps(task_handle);
 }
 
+TEST_CASE("IDF additions: Task creation with memory caps and self deletion", "[freertos]")
+{
+    TaskHandle_t task_handle = NULL;
+    StackType_t *puxStackBuffer;
+    StaticTask_t *pxTaskBuffer;
+
+    // Create a task with caps
+    TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCoreWithCaps(task_with_caps_self_delete, "task", 4096, (void *)xTaskGetCurrentTaskHandle(), UNITY_FREERTOS_PRIORITY + 1, &task_handle, UNITY_FREERTOS_CPU, OBJECT_MEMORY_CAPS));
+    TEST_ASSERT_NOT_EQUAL(NULL, task_handle);
+    // Get the task's memory
+    TEST_ASSERT_EQUAL(pdTRUE, xTaskGetStaticBuffers(task_handle, &puxStackBuffer, &pxTaskBuffer));
+    TEST_ASSERT(esp_ptr_in_dram(puxStackBuffer));
+    TEST_ASSERT(esp_ptr_in_dram(pxTaskBuffer));
+    // Notify the task to delete itself
+    xTaskNotifyGive(task_handle);
+}
+
+#if ( CONFIG_FREERTOS_NUMBER_OF_CORES > 1 )
+
+static void task_with_caps_running_on_other_core(void *arg)
+{
+    /* Notify the unity task that this task is running on the other core */
+    xTaskNotifyGive((TaskHandle_t)arg);
+
+    /* We make sure that this task is running on the other core */
+    while (1) {
+        ;
+    }
+}
+
+TEST_CASE("IDF additions: Task creation with memory caps and deletion from another core", "[freertos]")
+{
+    TaskHandle_t task_handle = NULL;
+    StackType_t *puxStackBuffer;
+    StaticTask_t *pxTaskBuffer;
+
+    // Create a task with caps on the other core
+    TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCoreWithCaps(task_with_caps_running_on_other_core, "task", 4096, (void *)xTaskGetCurrentTaskHandle(), UNITY_FREERTOS_PRIORITY + 1, &task_handle, !UNITY_FREERTOS_CPU, OBJECT_MEMORY_CAPS));
+    TEST_ASSERT_NOT_EQUAL(NULL, task_handle);
+    // Get the task's memory
+    TEST_ASSERT_EQUAL(pdTRUE, xTaskGetStaticBuffers(task_handle, &puxStackBuffer, &pxTaskBuffer));
+    TEST_ASSERT(esp_ptr_in_dram(puxStackBuffer));
+    TEST_ASSERT(esp_ptr_in_dram(pxTaskBuffer));
+    // Wait for the created task to start running
+    ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+    // Delete the task from another core
+    vTaskDeleteWithCaps(task_handle);
+}
+
+#endif // CONFIG_FREERTOS_NUMBER_OF_CORES > 1
+
 TEST_CASE("IDF additions: Queue creation with memory caps", "[freertos]")
 {
     QueueHandle_t queue_handle;