Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>
>>>>>>> -#define test_claimed(owner) ((long) (owner) & 1)
>>>>>>> -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
>>>>>>> -#define set_claimed(owner, bit) \
>>>>>>> -        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
>>>>>>> +#define __CLAIMED_BIT          XN_HANDLE_SPARE3
>>>>>>> +
>>>>>>> +#define test_claimed(owner)    xnhandle_test_spare(owner, 
>>>>>>> __CLAIMED_BIT)
>>>>>>> +#define clear_claimed(owner)   xnhandle_mask_spare(owner)
>>>>>>> +#define set_claimed(owner, bit) ({ \
>>>>>>> +       xnhandle_t __tmp = xnhandle_mask_spare(owner); \
>>>>>>> +       if (bit) \
>>>>>>> +               xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
>>>>>>> +       __tmp; \
>>>>>>> +})
>>>>>> I liked doing this with no conditional.
>>>>> You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
>>>>> would require some renaming then, I guess)?
>>>>>
>>>>>>> +       xnarch_atomic_set(mutex->owner,
>>>>>>> +                         set_claimed(xnthread_handle(owner),
>>>>>>> +                                     
>>>>>>> xnsynch_nsleepers(&mutex->synchbase)));
>>>>>> Ok. I think you have spotted a bug here. This should be mutex->sleepers
>>>>>> instead of xnsynch_nsleepers.
>>>>> OK. Will you fix? I will rebase then.
>>>>>
>>>>>>> +       /* Consistency check for owner handle - is the object a thread? 
>>>>>>> */
>>>>>>> +       if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
>>>>>>> +               err = -EINVAL;
>>>>>>> +               goto error;
>>>>>>>         }
>>>>>> What is this ?
>>>>> This ensures that we are not dereferences some arbitrary registry object
>>>>> due to a borken handle in the mutex variable: The the object registered
>>>>> on this handle is an xnthread, we will find the very same handle value
>>>>> at the well-known place in the object. Kind of magic for xnthread
>>>>> objects (instead of adding a magic field to them).
>>>> Yes, but the point is: how can the handle be borken ? It looks like a
>>>> useless check to me.
>>> This is user memory. Everything can be borken. And I don't want to
>>> create yet another source for user-triggered kernel bugs (including
>>> silent ones) like with our heap management structures that are
>>> corruptible by userland. That's headache guarantee.
>> No, we assume that the user application will only clobber our heap if it
>> is broken too. Add this with a XENO_DEBUG option if you want (not
>> XENO_DEBUG(POSIX)).
> 
> No problem for me. This check is almost 100% identical (specifically
> overhead-wise) to the usual object magic checks after querying the
> registry, and as those are unconditionally performed, I saw no need to
> handle this case differently.

But the registry lookup is already a validation in itself. I mean, if an
application uses 5 as a file descriptor, there are only two cases:
- the file descriptor 5 exists in the application and the kernel has no
other choice than to believe that the application is using this file
descriptor, and nothing will go wrong because this is a valid descriptor;
- the file descriptor does not exist, and we will return EBADF, and
nothing wrong will happen either...

-- 
                                                 Gilles.

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to