Re: [Xenomai-core] [PATCH 2/2] native: Fix error cleanup of rt_task_create

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

On 06/30/2011 01:32 PM, Jan Kiszka wrote:
> On 2011-06-30 13:09, Gilles Chanteperdrix wrote:
>> On 06/30/2011 11:36 AM, Jan Kiszka wrote:
>>> 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.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>> (...)
>>> +
>>> +      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.
> 
> 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.

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.

-- 
                                                                Gilles.