native: Fix error cleanup of rt_task_create
PATCH 2/2 native: Fix error cleanup of rt_task_create
When creating of a shadow task fails, rt_task_create has to free the task object consistently, not only on registry errors. Then we need to delete the core thread when fastlock allocation failed. Moreover, fix a double free of the fastlock object which is now released via the delete hook. Finally, avoid a use-after-release of the fastlock object in __task_delete_hook.
This fixes heap corruptions when running out of resources.
---
Applies on top of xenomai-gch.git gch/u_mode
ksrc/skins/native/syscall.c | 4 ++- ksrc/skins/native/task.c | 44 ++++++++++++++++++++++++------------------ 2 files changed, 28 insertions(+), 20 deletions(-)
ksrc/skins/native/syscall.c +3 -1
@@ -184,8 +184,10 @@ static int __rt_task_create(struct pt_regs *regs)the platform does not support it. */ err =rt_task_create (task, name,0 , prio,XNFPU |XNSHADOW | mode); -if (err) +if (err) { + task =NULL ; goto fail; + }/* Apply CPU affinity */ set_cpus_allowed (p, task->affinity);
ksrc/skins/native/task.c +25 -19
88 lines not on this page · raw message
Signature · 1 line
1.7.1
Quoted 12 lines · Jan Kiszka
When creating of a shadow task fails,
rt_task_createhas to free the task object consistently, not only on registry errors. Then we need to delete the core thread when fastlock allocation failed. Moreover, fix a double free of the fastlock object which is now released via the delete hook. Finally, avoid a use-after-release of the fastlock object in__task_delete_hook.This fixes heap corruptions when running out of resources.
Signed-off-by Jan Kiszka <[email protected]>---
(...)
ksrc/skins/native/task.c int rt_task_create(RT_TASK *task, · line 356
Quoted 9 lines
ksrc/skins/native/task.c +6
-0+ + fail: +if (xnthread_test_state (&task->thread_base,XNSHADOW )) +xnfree (task); + +return err; }/**
Is this needed? I mean, shadows are created in syscall.c, function __rt_task_create, and when rt_task_create returns an error, that function calls rt_task_delete. So, there should be no leak. And worse, here rt_task_delete will use an invalid pointer if we apply that patch.
Signature · 1 line
Gilles.
Quoted 21 lines · Gilles Chanteperdrix
Quoted 20 lines · Jan Kiszka
When creating of a shadow task fails,
rt_task_createhas to free the task object consistently, not only on registry errors. Then we need to delete the core thread when fastlock allocation failed. Moreover, fix a double free of the fastlock object which is now released via the delete hook. Finally, avoid a use-after-release of the fastlock object in__task_delete_hook.This fixes heap corruptions when running out of resources.
Signed-off-by Jan Kiszka <[email protected]>---
(...)ksrc/skins/native/task.c +6
-0+ + fail: +if (xnthread_test_state (&task->thread_base,XNSHADOW )) +xnfree (task); + +return err; }
Quoted 6 lines
Quoted 1 line
/**
Is this needed? I mean, shadows are created in
syscall.c, function__rt_task_create, and whenrt_task_createreturns an error, that function callsrt_task_delete. So, there should be no leak. And worse, herert_task_deletewill use an invalid pointer if we apply that patch.
The problem is that rt_task_create may both fail early without any registration step performed yet and very late when everything (except the the registry entry) is already set up. There is no chance for the caller __rt_task_create to figure out the precise task registration state and properly roll it back. That must be done by rt_task_create.
This patch ensures that shadow tasks are either successfully registered or completely deleted on error. Among other bug scenarios, this avoids deleting the task twice, first via xnpod_delete_thread+deletion hook and then again via xnfree in the error path of __rt_task_create.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 35 lines · Jan Kiszka
Quoted 21 lines · Gilles Chanteperdrix
Quoted 20 lines · Jan Kiszka
When creating of a shadow task fails,
rt_task_createhas to free the task object consistently, not only on registry errors. Then we need to delete the core thread when fastlock allocation failed. Moreover, fix a double free of the fastlock object which is now released via the delete hook. Finally, avoid a use-after-release of the fastlock object in__task_delete_hook.This fixes heap corruptions when running out of resources.
Signed-off-by Jan Kiszka <[email protected]>---
(...)ksrc/skins/native/task.c +6
-0+ + fail: +if (xnthread_test_state (&task->thread_base,XNSHADOW )) +xnfree (task); + +return err; }Quoted 6 lines
Quoted 1 line
/**
Is this needed? I mean, shadows are created in
syscall.c, function__rt_task_create, and whenrt_task_createreturns an error, that function callsrt_task_delete. So, there should be no leak. And worse, herert_task_deletewill use an invalid pointer if we apply that patch.The problem is that
rt_task_createmay both fail early without any registration step performed yet and very late when everything (except the the registry entry) is already set up. There is no chance for the caller__rt_task_createto figure out the precise task registration state and properly roll it back. That must be done byrt_task_create.
Quoted 4 lines
This patch ensures that shadow tasks are either successfully registered or completely deleted on error. Among other bug scenarios, this avoids deleting the task twice, first via xnpod_delete_thread+deletion hook and then again via xnfree in the error path of
__rt_task_create.
I need to understand completely this issue. But I definitely prefer seeing the xnfree in the same function as xnmalloc in case of failure. This makes chasing leaks easier.
Signature · 1 line
Gilles.