[Xenomai-core] Fix rt_task_shadow error path without __thread.

[Xenomai-core] Fix rt_task_shadow error path without __thread.

[Gilles Chanteperdrix]

Hi,

it seems that rt_task_shadow currently leaves the self tsd assigned (and
uninitialized) in case of error. So, here is an attempt to fix this
situation:

Index: src/skins/native/task.c
===================================================================
--- src/skins/native/task.c     (revision 4727)
+++ src/skins/native/task.c     (working copy)
@@ -187,15 +187,8 @@

 #ifdef HAVE___THREAD
        self = &__native_self;
-#else /* !HAVE___THREAD */
-       self = pthread_getspecific(__native_tskey);
+#endif /* HAVE___THREAD */

-       if (!self)
-               self = malloc(sizeof(*self));
-
-       pthread_setspecific(__native_tskey, self);
-#endif /* !HAVE___THREAD */
-
        if (task == NULL)
                task = &task_desc; /* Discarded. */

@@ -217,9 +210,13 @@
                                NULL);

        if (!err) {
-               if (self)
-                       *self = *task;
+#ifndef HAVE___THREAD
+               self = malloc(sizeof(*self));

+               pthread_setspecific(__native_tskey, self);
+#endif /* !HAVE___THREAD */
+               *self = *task;
+
                xeno_set_current();

                if (mode & T_WARNSW)


-- 
                                                 Gilles.

Re: [Xenomai-core] Fix rt_task_shadow error path without __thread.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

Gilles Chanteperdrix wrote:
> Hi,
> 
> it seems that rt_task_shadow currently leaves the self tsd assigned (and
> uninitialized) in case of error. So, here is an attempt to fix this
> situation:

Good point.

> 
> Index: src/skins/native/task.c
> ===================================================================
> --- src/skins/native/task.c     (revision 4727)
> +++ src/skins/native/task.c     (working copy)
> @@ -187,15 +187,8 @@
> 
>  #ifdef HAVE___THREAD
>         self = &__native_self;
> -#else /* !HAVE___THREAD */
> -       self = pthread_getspecific(__native_tskey);
> +#endif /* HAVE___THREAD */
> 
> -       if (!self)
> -               self = malloc(sizeof(*self));
> -
> -       pthread_setspecific(__native_tskey, self);
> -#endif /* !HAVE___THREAD */
> -
>         if (task == NULL)
>                 task = &task_desc; /* Discarded. */
> 
> @@ -217,9 +210,13 @@
>                                 NULL);
> 
>         if (!err) {
> -               if (self)
> -                       *self = *task;
> +#ifndef HAVE___THREAD
> +               self = malloc(sizeof(*self));

Hmm, this creates a potential relaxation point (and may raise SIGXCPU if
the warn flag is set). I would prefer to keep early allocation and
release the chunk properly on error (as well as reset the tsd).

> 
> +               pthread_setspecific(__native_tskey, self);
> +#endif /* !HAVE___THREAD */
> +               *self = *task;

And if self is NULL, ie. malloc failed for whatever obscure reasons? The
existing code skipped over this, maybe we should actually handle it and
return an error code.

> +
>                 xeno_set_current();
> 
>                 if (mode & T_WARNSW)
> 
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] Fix rt_task_shadow error path without __thread.

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Hi,
>>
>> it seems that rt_task_shadow currently leaves the self tsd assigned (and
>> uninitialized) in case of error. So, here is an attempt to fix this
>> situation:
> 
> Good point.
> 
>> Index: src/skins/native/task.c
>> ===================================================================
>> --- src/skins/native/task.c     (revision 4727)
>> +++ src/skins/native/task.c     (working copy)
>> @@ -187,15 +187,8 @@
>>
>>  #ifdef HAVE___THREAD
>>         self = &__native_self;
>> -#else /* !HAVE___THREAD */
>> -       self = pthread_getspecific(__native_tskey);
>> +#endif /* HAVE___THREAD */
>>
>> -       if (!self)
>> -               self = malloc(sizeof(*self));
>> -
>> -       pthread_setspecific(__native_tskey, self);
>> -#endif /* !HAVE___THREAD */
>> -
>>         if (task == NULL)
>>                 task = &task_desc; /* Discarded. */
>>
>> @@ -217,9 +210,13 @@
>>                                 NULL);
>>
>>         if (!err) {
>> -               if (self)
>> -                       *self = *task;
>> +#ifndef HAVE___THREAD
>> +               self = malloc(sizeof(*self));
> 
> Hmm, this creates a potential relaxation point (and may raise SIGXCPU if
> the warn flag is set). I would prefer to keep early allocation and
> release the chunk properly on error (as well as reset the tsd).
> 
>> +               pthread_setspecific(__native_tskey, self);
>> +#endif /* !HAVE___THREAD */
>> +               *self = *task;
> 
> And if self is NULL, ie. malloc failed for whatever obscure reasons? The
> existing code skipped over this, maybe we should actually handle it and
> return an error code.

Ok. Here is a new version which addresses the above critics. Note that 
there is still a potential for SIGXCPU: if the task calling 
rt_task_shadow has already been mapped by another skin, the 
__native_task_create syscall will fail, and free will be called. 
However, such use of rt_task_shadow may be considered a programming 
error, and we may accept a SIGXCPU in such a case.

diff --git a/src/skins/native/task.c b/src/skins/native/task.c
index 905d366..e448576 100644
--- a/src/skins/native/task.c
+++ b/src/skins/native/task.c
@@ -188,10 +188,13 @@ int rt_task_shadow(RT_TASK *task, const char *name, int prio
 #ifdef HAVE___THREAD
        self = &__native_self;
 #else /* !HAVE___THREAD */
-       self = pthread_getspecific(__native_tskey);
+       if (pthread_getspecific(__native_tskey))
+               /* Current task is already a native taks. */
+               return -EBUSY;

+       self = malloc(sizeof(*self));
        if (!self)
-               self = malloc(sizeof(*self));
+               return -ENOMEM;

        pthread_setspecific(__native_tskey, self);
 #endif /* !HAVE___THREAD */
@@ -210,21 +213,30 @@ int rt_task_shadow(RT_TASK *task, const char *name, int prio
        bulk.a5 = (u_long)pthread_self();
        bulk.a6 = (u_long)xeno_init_current_mode();

-       if (!bulk.a6)
-               return -ENOMEM;
+       if (!bulk.a6) {
+               err = -ENOMEM;
+               goto fail;
+       }

        err = XENOMAI_SKINCALL2(__native_muxid, __native_task_create, &bulk,
                                NULL);
+       if (err)
+               goto fail;

-       if (!err) {
-               if (self)
-                       *self = *task;
+       *self = *task;

-               xeno_set_current();
+       xeno_set_current();

-               if (mode & T_WARNSW)
-                       xeno_sigxcpu_no_mlock = 0;
-       }
+       if (mode & T_WARNSW)
+               xeno_sigxcpu_no_mlock = 0;
+
+       return 0;
+
+  fail:
+#ifndef HAVE___THREAD
+       pthread_setspecific(__native_tskey, NULL);
+       free(self);
+#endif  /* !HAVE___THREAD */

        return err;
 }


-- 
					    Gilles.

Re: [Xenomai-core] Fix rt_task_shadow error path without __thread.

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Hi,
>>>
>>> it seems that rt_task_shadow currently leaves the self tsd assigned (and
>>> uninitialized) in case of error. So, here is an attempt to fix this
>>> situation:
>> Good point.
>>
>>> Index: src/skins/native/task.c
>>> ===================================================================
>>> --- src/skins/native/task.c     (revision 4727)
>>> +++ src/skins/native/task.c     (working copy)
>>> @@ -187,15 +187,8 @@
>>>
>>>  #ifdef HAVE___THREAD
>>>         self = &__native_self;
>>> -#else /* !HAVE___THREAD */
>>> -       self = pthread_getspecific(__native_tskey);
>>> +#endif /* HAVE___THREAD */
>>>
>>> -       if (!self)
>>> -               self = malloc(sizeof(*self));
>>> -
>>> -       pthread_setspecific(__native_tskey, self);
>>> -#endif /* !HAVE___THREAD */
>>> -
>>>         if (task == NULL)
>>>                 task = &task_desc; /* Discarded. */
>>>
>>> @@ -217,9 +210,13 @@
>>>                                 NULL);
>>>
>>>         if (!err) {
>>> -               if (self)
>>> -                       *self = *task;
>>> +#ifndef HAVE___THREAD
>>> +               self = malloc(sizeof(*self));
>> Hmm, this creates a potential relaxation point (and may raise SIGXCPU if
>> the warn flag is set). I would prefer to keep early allocation and
>> release the chunk properly on error (as well as reset the tsd).
>>
>>> +               pthread_setspecific(__native_tskey, self);
>>> +#endif /* !HAVE___THREAD */
>>> +               *self = *task;
>> And if self is NULL, ie. malloc failed for whatever obscure reasons? The
>> existing code skipped over this, maybe we should actually handle it and
>> return an error code.
> 
> Ok. Here is a new version which addresses the above critics. Note that 
> there is still a potential for SIGXCPU: if the task calling 
> rt_task_shadow has already been mapped by another skin, the 
> __native_task_create syscall will fail, and free will be called. 
> However, such use of rt_task_shadow may be considered a programming 
> error, and we may accept a SIGXCPU in such a case.

Agreed.

> 
> diff --git a/src/skins/native/task.c b/src/skins/native/task.c
> index 905d366..e448576 100644
> --- a/src/skins/native/task.c
> +++ b/src/skins/native/task.c
> @@ -188,10 +188,13 @@ int rt_task_shadow(RT_TASK *task, const char *name, int prio
>  #ifdef HAVE___THREAD
>         self = &__native_self;
>  #else /* !HAVE___THREAD */
> -       self = pthread_getspecific(__native_tskey);
> +       if (pthread_getspecific(__native_tskey))
> +               /* Current task is already a native taks. */
> +               return -EBUSY;
> 
> +       self = malloc(sizeof(*self));
>         if (!self)
> -               self = malloc(sizeof(*self));
> +               return -ENOMEM;
> 
>         pthread_setspecific(__native_tskey, self);
>  #endif /* !HAVE___THREAD */
> @@ -210,21 +213,30 @@ int rt_task_shadow(RT_TASK *task, const char *name, int prio
>         bulk.a5 = (u_long)pthread_self();
>         bulk.a6 = (u_long)xeno_init_current_mode();
> 
> -       if (!bulk.a6)
> -               return -ENOMEM;
> +       if (!bulk.a6) {
> +               err = -ENOMEM;
> +               goto fail;
> +       }
> 
>         err = XENOMAI_SKINCALL2(__native_muxid, __native_task_create, &bulk,
>                                 NULL);
> +       if (err)
> +               goto fail;
> 
> -       if (!err) {
> -               if (self)
> -                       *self = *task;
> +       *self = *task;
> 
> -               xeno_set_current();
> +       xeno_set_current();
> 
> -               if (mode & T_WARNSW)
> -                       xeno_sigxcpu_no_mlock = 0;
> -       }
> +       if (mode & T_WARNSW)
> +               xeno_sigxcpu_no_mlock = 0;
> +
> +       return 0;
> +
> +  fail:
> +#ifndef HAVE___THREAD
> +       pthread_setspecific(__native_tskey, NULL);
> +       free(self);
> +#endif  /* !HAVE___THREAD */
> 
>         return err;
>  }
> 

Looks good to me.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux