platform:fix possible double free for FreeRTOS #98
Conversation
|
This is not correct and causes memory leak. vTaskDelete frees only pointer in field thread thread which is allocated in by xTaskCreate and platform_memory_free(thread) releases pointer to thread structure which allocated by |
|
but platform_memory_free is called again after destroy here mqttclient/mqttclient/mqttclient.c Lines 959 to 960 in 956e0c8 So thread storage variable seems to be freed twice. |
|
Oh, indeed. There refactoring is required. Allocation is done inside the platform and free is outside. It is responsibility of platform_thread_destroy to free structure because it is allocated there. So we need to remove free of thread structure in mqttclient and add free inside destroy in other ports |
|
yes, either way, the two free has to be prevented. and yes ideally platform code needs to take care of allocation and free. Shall I make necessary change and push again? |
|
however, if you look at other implementations e.g. TencentOS, thread variable is not being freed. so other platforms are to modified too. mqttclient/platform/TencentOS-tiny/platform_thread.c Lines 58 to 64 in 956e0c8 |
|
Yes, let's do it in the right way without creating new tangling. Where variable is allocated, there it needs to be freed |
Signed-off-by: Ajay Bhargav <ajay.bhargav@waybyte.in>
This patch fix the issue where platform_thread_destroy do not actually delete the thread created, only memory free was done. Signed-off-by: Ajay Bhargav <ajay.bhargav@waybyte.in>
ajaybhargav
force-pushed
the
memory_free_issue
branch
from
7355bd3
to
5a51b21
Compare
|
Another issue found in Tencent-OS implementation, stk_base needed to be freed not stk_size. Please look into the new patches |
| @@ -60,7 +60,8 @@ void platform_thread_destroy(platform_thread_t* thread) | |||
| if (NULL != thread) | |||
| tos_task_destroy(&(thread->thread)); | |||
| platform_memory_free(&(thread->thread)); | |||
There was a problem hiding this comment.
&(thread->thread) is equal to thread. This line is need to be removed. There are only two allocations: thread and stack
There was a problem hiding this comment.
Yes, so the order need to be changed, after thread memory is free, it is not advisable to access thread structure.
In tencentos implementation of platform_thread_destroy, stack size is getting freed where as stack base is suppose to be freed. platform_thread_t structure varaible is also not freed. this patch fix both the issues Signed-off-by: Ajay Bhargav <ajay.bhargav@waybyte.in>
Signed-off-by: Ajay Bhargav <ajay.bhargav@waybyte.in>
ajaybhargav
force-pushed
the
memory_free_issue
branch
from
5a51b21
to
c869ebb
Compare
|
Updated, please review. |
FreeRTOS implementation of platform_thread_destroy free the pointer passed to the function and later the same pointer is freed again at mqttclient/mqttclient.c#L959
This will cause possible double free for FreeRTOS implementation.