From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4E12148C.7050500@domain.hid> Date: Mon, 04 Jul 2011 21:29:16 +0200 From: Gilles Chanteperdrix MIME-Version: 1.0 References: <4E0C439D.2030602@domain.hid> <4E0C5979.7020202@domain.hid> <4E0C5EC4.9050900@domain.hid> In-Reply-To: <4E0C5EC4.9050900@domain.hid> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [PATCH 2/2] native: Fix error cleanup of rt_task_create List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Xenomai core 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 >>> --- >>> (...) >>> + >>> + 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.