This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: pthread_mutex_unlock potentially cause invalid access


2012/2/13 Carlos O'Donell <carlos@systemhalted.org>:
> On Mon, Feb 13, 2012 at 9:22 AM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com> wrote:
>> 2012/2/13 Atsushi Nemoto <anemo@mba.ocn.ne.jp>:
>>> On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <carlos@systemhalted.org> wrote:
>>>>> On point "A", the mutex is actually unlocked, so other threads can
>>>>> lock the mutex, unlock, destroy and free. ?If the mutex was destroyed
>>>>> and freed by other thread, reading '__kind' on point "B" is not valid.
>>>>
>>>> No valid conforming program has this behaviour.
>>> ...
>>>> If {X-X'} need to be prevented from using M then you need to use
>>>> *another* synchronization primitive, we call it P, to prevent the use
>>>> M during the destruction of M.
>>>
>>> How about following example in pthread_mutex_destroy manual?
>>> It seems it can be possible without P.
>>>
>>> http://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_destroy.html
>>> ------------------------------------------------------------------------
>>> ? ?Destroying Mutexes
>>>
>>> ? ?A mutex can be destroyed immediately after it is unlocked. For
>>> ? ?example, consider the following code:
>>>
>>> ? ?struct obj {
>>> ? ?pthread_mutex_t om;
>>> ? ? ? ?int refcnt;
>>> ? ? ? ?...
>>> ? ?};
>>>
>>> ? ?obj_done(struct obj *op)
>>> ? ?{
>>> ? ? ? ?pthread_mutex_lock(&op->om);
>>> ? ? ? ?if (--op->refcnt == 0) {
>>> ? ? ? ? ? ?pthread_mutex_unlock(&op->om);
>>> ? ?(A) ? ? pthread_mutex_destroy(&op->om);
>>> ? ?(B) ? ? free(op);
>>> ? ? ? ?} else
>>> ? ?(C) ? ? pthread_mutex_unlock(&op->om);
>>> ? ?}
>>>
>>> ? ?In this case obj is reference counted and obj_done() is called
>>> ? ?whenever a reference to the object is dropped. Implementations are
>>> ? ?required to allow an object to be destroyed and freed and potentially
>>> ? ?unmapped (for example, lines A and B) immediately after the object is
>>> ? ?unlocked (line C).
>>> ------------------------------------------------------------------------
>>>
>>> In this example, (A) and (B) can be executed in middle of (C) execution.
>>> Thus, problem can be happen even on a fully conforming program, no?
>>
>> Heh, Interesting. I think you are right.
>>
>> At first, I thought we don't need to fix this likes Carlos because the
>> standard says
>>
>> http://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_destroy.html
>>
>> pthread_mutex_destroy:
>> ? It shall be safe to destroy an initialized mutex that is unlocked.
>> Attempting to destroy a locked mutex results in undefined behavior.
>>
>> So, while thread T is under mutex_unlock, any other thread shouldn't
>> call pthread_mutex_destroy(). Thus, current implementation fullfill a
>> requirement of the standard. But, yes, it also violate an example code
>> of the standard. iow, SUS is suck.
>>
>> And, now lll_unlock has costly atomic operation (A) and syscall (B),
>
> The atomic operation and syscall are *required* to implement the
> semantics of a futex.

Right.


>>> #define lll_unlock(lock, private) \
>>> ?((void) ({ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>>> ? ?int *__futex = &(lock); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>>> ? ?int __val = atomic_exchange_rel (__futex, 0); ? ? ? ? ? ? \ ?(A)
>>> ? ?if (__builtin_expect (__val > 1, 0)) ? ? ? ? ? ? ? ? ? ? ?\
>>> ? ? ?lll_futex_wake (__futex, 1, private); ? ? ? ? ? ? ? ? ? \ ? (B)
>>> ?}))
>>
>> thus, a few register move are much less costly than that. Plus,
>> userland application developers hope non-x86 code works the same as
>> x86. so, I think your proposal has good benefit/cost ratio.
>
> I don't understand your suggestion.
>
> If you have a suggestion for an enhancement please file a bugzilla
> with a patch and we can talk about the benefits.

I think the bug reporter should do.


> Adding instructions to the fast path to reduce the possibility of a
> segfault in a application with problems is not a good benefit/cost.
>
> Cheers,
> Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]