[BUG] - TCBs and dynamic stacks leaked when scheduler is stopped

[cmorganBE]

When the scheduler is stopped, TCBs and dynamic stacks of running threads are not being freed. While ports, such as the posix port, clean up their resources, it appears as if the kernel itself is not freeing these resources when the scheduler is stopped.

TCBs are leaked from here, tasks.c: 1670

                /* Allocate space for the TCB. */
                /* MISRA Ref 11.5.1 [Malloc memory assignment] */
                /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */
                /* coverity[misra_c_2012_rule_11_5_violation] */
                pxNewTCB = ( TCB_t * ) pvPortMalloc( sizeof( TCB_t ) );  <----------------------------

Stacks are leaked from here, tasks.c:1662

            /* MISRA Ref 11.5.1 [Malloc memory assignment] */
            /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */
            /* coverity[misra_c_2012_rule_11_5_violation] */
            pxStack = pvPortMallocStack( ( ( ( size_t ) usStackDepth ) * sizeof( StackType_t ) ) );

            if( pxStack != NULL )
            {

@chinglee-iot with the merge of the posix changes I'm making use of them here for some tests. I'm not sure how it was missed before but it looks like we may have missed leaking task TCBs and dynamic task stacks.

A task's tcb appears to be freed in prvDeleteTCB() but its unclear how the posix port changes of cancelling and
deleting threads accomplish this.

Should vTaskStartScheduler() delete the TCBs and dynamic stacks for any running threads before returning? It feels like the port shouldn't really be doing this since TCBs/dynamic stacks are created and managed by FreeRTOS kernel proper.

Valgrind trace that got me started:

==1415351== HEAP SUMMARY:
==1415351==     in use at exit: 37,088 bytes in 4 blocks
==1415351==   total heap usage: 2,506 allocs, 2,502 frees, 491,264 bytes allocated
==1415351== 
==1415351== 160 bytes in 1 blocks are possibly lost in loss record 2 of 4
==1415351==    at 0x4845828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1415351==    by 0x1F0BB6: pvPortMalloc (heap_3.c:65) <---------------- this is a TCB
==1415351==    by 0x1EB38D: prvCreateTask (tasks.c:1670)
==1415351==    by 0x1EB460: xTaskCreate (tasks.c:1722)
==1415351==    by 0x1E6405: WaterTemperatureSim::Enable() (WaterTemperatureSim.h:51)
==1415351==    by 0x1E59D1: WaterTemperatureSimTest(void*) (test_WaterTemperatureSim.cpp:23)
==1415351==    by 0x1F1442: prvWaitForStart (port.c:507)
==1415351==    by 0x4C8CAD9: start_thread (pthread_create.c:444)
==1415351==    by 0x4D1D2E3: clone (clone.S:100)
==1415351== 
==1415351== 4,000 bytes in 1 blocks are possibly lost in loss record 3 of 4
==1415351==    at 0x4845828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1415351==    by 0x1F0BB6: pvPortMalloc (heap_3.c:65) <---------------- this is a task dynamic stack
==1415351==    by 0x1EB378: prvCreateTask (tasks.c:1662)
==1415351==    by 0x1EB460: xTaskCreate (tasks.c:1722)
==1415351==    by 0x1E6405: WaterTemperatureSim::Enable() (WaterTemperatureSim.h:51)
==1415351==    by 0x1E59D1: WaterTemperatureSimTest(void*) (test_WaterTemperatureSim.cpp:23)
==1415351==    by 0x1F1442: prvWaitForStart (port.c:507)
==1415351==    by 0x4C8CAD9: start_thread (pthread_create.c:444)
==1415351==    by 0x4D1D2E3: clone (clone.S:100)

[chinglee-iot]

@cmorganBE
Thank you for reporting back. I will take a look at this issue.

[cmorganBE]

@chinglee-iot and to be clear, the call to pvPortMallocStack() is also leaking, this one:

tasks.c:1662

            /* MISRA Ref 11.5.1 [Malloc memory assignment] */
            /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */
            /* coverity[misra_c_2012_rule_11_5_violation] */
            pxStack = pvPortMallocStack( ( ( ( size_t ) usStackDepth ) * sizeof( StackType_t ) ) );

            if( pxStack != NULL )
            {

[cmorganBE]

@chinglee-iot did you want me to look at this one? Would deleting TCBs and stacks when the scheduler is exiting be an acceptable change that could be viable for a merge?

[Skptak]

Hey @cmorganBE, I think that the proposed idea of deleting all tasks when stopping the scheduler is a change that should be discussed a bit more?

I'm thinking about a use case where somebody wants to stop the scheduler, create and delete a few tasks perhaps, and then restart it

Would you mind making a post on the FreeRTOS Forum about this so we can discuss it more there?

[chinglee-iot]

@cmorganBE

I've been blocked by other stuff and now get time to look into this problem.

The callstack you shared indicates that memory dynamically allocated for task are not freed, include TCB_t and stack. Allowing to delete those tasks after scheduler is stopped is one way to free those memory. We are looking into the implementation of vTaskDelete() to see if any change required.

The other problem is that we should call vTaskDelete() to delete all the tasks, including created by application and kernel. kernel created task handles can be obtained with xTaskGetIdleTaskHandle() and xTimerGetTimerDaemonTaskHandle(). That means application should keep all the task handles and delete them after scheduler stopped or we need another method to delete those tasks.

To free memory after scheduler stopped is not supported before. It is more like a new feature to FreeRTOS. As @Skptak suggested, we can also discuss it more in the forum.

[cmorganBE]

@Skptak @chinglee-iot

@chinglee-iot imo its inconsistent if we require the user of FreeRTOS To free these resources as the POSIX port already frees the internal ones and the official FreeRTOS docs say the kernel will free the rest, https://www.freertos.org/a00133.html.

Recent changes for the POSIX port perform cleanup when the scheduler is stopped so we've already freed the pthread threads, its the FreeRTOS resources that are leaking. I would be fine with either approach:

A. Scheduler is stopped and all resources are cleaned up (today POSIX port does this but FreeRTOS proper does not)
B. Scheduler can be stopped/started and there is a separate way to clean up resources. This approach has to involve some interface for the port as there are port specific resources, unrelated to FreeRTOS tasks, that need to be cleaned up.

At the moment we are close to having 'A' completed if FreeRTOS freed TCBs and stacks on scheduler stop.

Also, 'A' is what the official docs say the behavior should be, https://www.freertos.org/a00133.html. Although this is only for the x86 port.

Again, I'm ok with either A or B but I worry we'll get stuck discussing it when we could fairly complete 'A' and let someone else interested in B discuss and propose how to do that, since it will need new API in the kernel proper.

[Skptak]

@cmorganBE

Again, I'm ok with either A or B but I worry we'll get stuck discussing it when we could fairly complete 'A' and let someone else interested in B discuss and propose how to do that, since it will need new API in the kernel proper.

That's a very good point, in my opinion. No need to let "perfect" be the enemy of good here. As you've pointed out, we can always add to this feature once it is implemented.

I think for now making it so that vTaskEndScheduler() implements a loop that deletes all FreeRTOS Tasks on the system, and frees any that are dynamically allocated, is a good place to start with this.

@chinglee-iot @aggarg what are your thoughts on this?

[Skptak]

I wonder if clearing out the TCBs and stacks of statically allocated tasks would also provide a somewhat generic method of "deleting" those tasks as well?

[chinglee-iot]

I think for now making it so that vTaskEndScheduler() implements a loop that deletes all FreeRTOS Tasks on the system, and frees any that are dynamically allocated, is a good place to start with this.

If this is for testing with simulator purpose, we can do this in vTaskEndScheduler(). Tasks created by application should still be deleted in application if not required anymore. Otherwise, it will still cause memory leakage. vTaskEndScheduler() should only delete those long running tasks which won't be deleted in real target.

vTaskDelete() can be used to delete task after scheduler stopped. Statically allocated task can also be deleted with this function. The task calling vTaskEndScheduler() which is pointed by pxCurrentTCB needs to be handled specially in the port.

[cmorganBE]

I think for now making it so that vTaskEndScheduler() implements a loop that deletes all FreeRTOS Tasks on the system, and frees any that are dynamically allocated, is a good place to start with this.

If this is for testing with simulator purpose, we can do this in vTaskEndScheduler(). Tasks created by application should still be deleted in application if not required anymore. Otherwise, it will still cause memory leakage. vTaskEndScheduler() should only delete those long running tasks which won't be deleted in real target.

@chinglee-iot it does sound like these are two different things. If https://www.freertos.org/a00133.html is followed and vTaskEndScheduler() deletes all tasks and cleans up memory then it will both be closer to the documentation and happens to resolve the leak when using the POSIX simulator.

It sounds like there may be reasons to end the scheduler in a non-POSIX port context but even in that case we'd be better aligning the kenel with the documented behavior of vTaskEndScheduler() in https://www.freertos.org/a00133.html.

[aggarg]

There are 2 type of kernel objects -

1. Kernel Objects created by the application - This includes tasks, queues, semaphores etc. created by the application.
2. Kernel Objects created by the kernel - This includes idle and timer task.

In order to keep the resource ownership clear, I think the application should delete the kernel objects that it creates and the kernel should delete the ones it creates. This probably means that we need to update our documentation as well.

@cmorganBE Would it solve your problem? With this, you can delete all the kernel objects (including tasks) in your test harness before calling the vTaskEndScheduler() and valgrind should report no memory leak.