Skip to content

Commit

Permalink
fix(freertos): Fixed memory leak issue in vTaskDeleteWithCaps()
Browse files Browse the repository at this point in the history
vTaskDeleteWithCaps() leaked memory when a task uses the API to delete
itself. This commit adds a fix to avoid the memory leak.

Closes #14222
  • Loading branch information
sudeep-mohanty committed Jul 26, 2024
1 parent 7806aeb commit c3da2ac
Show file tree
Hide file tree
Showing 5 changed files with 207 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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" )
Expand Down
Original file line number Diff line number Diff line change
@@ -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
*/
Expand Down Expand Up @@ -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 );
Expand Down
131 changes: 120 additions & 11 deletions components/freertos/esp_additions/idf_additions.c
Original file line number Diff line number Diff line change
@@ -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
*/
Expand All @@ -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 ------------------------------------------- */

Expand Down Expand Up @@ -81,21 +83,128 @@

#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 );

/* 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();
}
}

/* Free the memory buffers */
heap_caps_free( puxStackBuffer );
vPortFree( pxTaskBuffer );
#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 ) */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
65 changes: 63 additions & 2 deletions components/freertos/test_apps/freertos/misc/test_idf_additions.c
Original file line number Diff line number Diff line change
@@ -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
*/
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down

0 comments on commit c3da2ac

Please sign in to comment.