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


On Fri, Feb 10, 2012 at 9:27 AM, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> It seems pthread_mutex_unlock() potentially cause invalid access on
> most platforms (except for i386 and x86_64).
>
> In nptl/pthread_mutex_unlock.c, lll_unlock() is called like this:
> ? ? ?lll_unlock (mutex->__data.__lock, PTHREAD_MUTEX_PSHARED (mutex));
>
> And PTHREAD_MUTEX_PSHARED() is defined like this:
> # define PTHREAD_MUTEX_PSHARED(m) \
> ?((m)->__data.__kind & 128)
>
> On most platforms, lll_unlock() is defined as a macro like this:
> #define lll_unlock(lock, private) \
> ?((void) ({ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ?int *__futex = &(lock); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ?int __val = atomic_exchange_rel (__futex, 0); ? ? ? ? ? ? \
> ? ?if (__builtin_expect (__val > 1, 0)) ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ?lll_futex_wake (__futex, 1, private); ? ? ? ? ? ? ? ? ? \
> ?}))
>
> Thus, the lll_unlock() call in pthread_mutex_unlock.c will be expanded as:
> ? ?int *__futex = &(mutex->__data.__lock);
> ? ?int __val = atomic_exchange_rel (__futex, 0);
> ? ?if (__builtin_expect (__val > 1, 0)) ? ? ? ? ? ? ? ?/* A */
> ? ? ?lll_futex_wake (__futex, 1, ((mutex)->__data.__kind & 128)); /* B */
>
> 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.

Let us call X the set of other threads.

Let us call Y the set of threads unlocking mutexes.

Let us call X' a thread in X, and Y' a thread in Y.

Let us call M the mutex that Y' is releasing.

X' can only know it is safe to destroy M IFF {X-X'} are not going to
use it, or {X-X'} are prevented from using M after Y' unlocks it.

If {X-X'} are not going to use M then you have no problem.

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.

If Y' is unlock M it must already have ownership of P and therefore X'
is waiting on Y' to complete the unlock after which Y' will release P
and allow X' to acquire it and destroy M.

Thus in a fully conforming program there is no problem.

If X' tries to delete M at point A then it is a non-conforming program
(deleting a locked mutex is undefined behaviour) because it might have
equally deleted M earlier because it has no way of synchronizing the
deletion with the unlock.

> Possible fix would be copying the 'private' argument to an internal
> local variable before atomic_exchange_rel(). ?Is it an appropriate fix?

Yes, that would make the code more robust, but it adds instructions to
the fast path (copying private argument).

However it doesn't fix the problem you present in the email which can
only be solved by knowing that no other thread will use the mutex (all
threads have been joined) or using another synchronization primitive
to ensure that mutex will not be used again.

Is it worth it to make the fast path slower for a fix that doesn't fix
the real problem?

I don't think it is unless someone comes up with a stronger example.

Cheers,
Carlos.


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