pthread_mutex_unlock potentially cause invalid access

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

pthread_mutex_unlock potentially cause invalid access

Atsushi Nemoto
It seems pthread_mutex_unlock() potentially cause invalid access on
most platforms (except for i386 and x86_64).

# Resend with correct ML address.  Excuse me for duplication.

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.

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

---
Atsushi Nemoto
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

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

That seems right to me off hand.  But I'm not really the expert on the
subtleties of this code.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Carlos O'Donell-2
In reply to this post by Atsushi Nemoto
On Fri, Feb 10, 2012 at 9:27 AM, Atsushi Nemoto <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Atsushi Nemoto
On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <[hidden email]> 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?

---
Atsushi Nemoto
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

KOSAKI Motohiro-2
2012/2/13 Atsushi Nemoto <[hidden email]>:

> On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <[hidden email]> 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),

> #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.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Carlos O'Donell-2
In reply to this post by Atsushi Nemoto
On Mon, Feb 13, 2012 at 8:01 AM, Atsushi Nemoto <[hidden email]> wrote:

> On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <[hidden email]> 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?

No. (A) and (B) can not be executed in the middle of (C).

In my previous example I stated that a synchronizing P would be
required to correctly support the destruction of the mutex.

In this example the synchronizing P is a reference counter.

The threads are serialized by the mutex and only the last thread,
using the reference counter, frees the object.

The assumption is that obj_done() is only called once per thread that
is releasing the object.

Note that the example avoids talking about how you increase the
reference count which is a much more complicated operation to do in a
thread-safe way.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Carlos O'Donell-2
In reply to this post by KOSAKI Motohiro-2
On Mon, Feb 13, 2012 at 9:22 AM, KOSAKI Motohiro
<[hidden email]> wrote:

> 2012/2/13 Atsushi Nemoto <[hidden email]>:
>> On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <[hidden email]> 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.

>> #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.

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.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Jakub Jelinek
In reply to this post by Carlos O'Donell-2
On Mon, Feb 13, 2012 at 09:39:34AM -0500, Carlos O'Donell wrote:

> >    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?
>
> No. (A) and (B) can not be executed in the middle of (C).

It can, the reporter is correct, the bug can be either fixed by ensuring in
the generic code (pthread_mutex_*lock.c etc.) that
lll_unlock/lll_robust_unlock is always
called with either constant or some automatic variable as second argument
(i.e. initialize a temporary from the currently used macros, then pass that
down), or by adjusting all target's lll_unlock/lll_robust_unlock macros,
or by turning them into inline functions.  The atomic operation will act as
a compiler/CPU barrier, so all that is needed is to make sure the read
in the source doesn't happen after the sync operation.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

KOSAKI Motohiro-2
In reply to this post by Carlos O'Donell-2
>>    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?
>
> No. (A) and (B) can not be executed in the middle of (C).

It can.
Look again, lll_unlock() call atomic_exchange_rel() _before_
(mutex)->__data dereference.

>   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 */


1) Thread-1) atomic_exchange_rel(0)
2) preempt
3) Thread-2) call mutex_lock(). (ok, it's success)
4) Thread-2) call mutex_unlock()
5) Thread-2) call mutex_destroy()
6) Thread-2) free(mutex)
7) preempt
8) Thread-3)  reuse memory of the mutex
9) preempt
10) Thread-1) dereference (mutex)->__data__.__kind

I don't think this example code makes much sense. but it is a part of
the standard. I believe people suspect libc honor the standard.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

KOSAKI Motohiro-2
In reply to this post by Carlos O'Donell-2
2012/2/13 Carlos O'Donell <[hidden email]>:

> On Mon, Feb 13, 2012 at 9:22 AM, KOSAKI Motohiro
> <[hidden email]> wrote:
>> 2012/2/13 Atsushi Nemoto <[hidden email]>:
>>> On Sun, 12 Feb 2012 17:38:33 -0500, "Carlos O'Donell" <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Carlos O'Donell-2
In reply to this post by KOSAKI Motohiro-2
On Mon, Feb 13, 2012 at 10:02 AM, KOSAKI Motohiro
<[hidden email]> wrote:

>>>    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?
>>
>> No. (A) and (B) can not be executed in the middle of (C).
>
> It can.
> Look again, lll_unlock() call atomic_exchange_rel() _before_
> (mutex)->__data dereference.
>
>>   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 */
>
>
> 1) Thread-1) atomic_exchange_rel(0)
> 2) preempt
> 3) Thread-2) call mutex_lock(). (ok, it's success)
> 4) Thread-2) call mutex_unlock()
> 5) Thread-2) call mutex_destroy()
> 6) Thread-2) free(mutex)
> 7) preempt
> 8) Thread-3)  reuse memory of the mutex
> 9) preempt
> 10) Thread-1) dereference (mutex)->__data__.__kind
>
> I don't think this example code makes much sense. but it is a part of
> the standard. I believe people suspect libc honor the standard.

Excellent, *this* makes sense and I now agree that there is a problem.

Nemoto-san could you please file a bug report and tag it with
glibc_2.15 keyword so I can backport this to 2.15?

As you suggest the fix is to copy __kind to a local variable.

I like Jakub's first suggestion to fix this in the generic code and
pass down the local/automatic to the call to
lll_unlock/lll_robust_unlock.

Needing to change all of the target code would be more work.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Atsushi Nemoto
On Mon, 13 Feb 2012 10:28:49 -0500, "Carlos O'Donell" <[hidden email]> wrote:
> Excellent, *this* makes sense and I now agree that there is a problem.
>
> Nemoto-san could you please file a bug report and tag it with
> glibc_2.15 keyword so I can backport this to 2.15?

Yes, I filed a bug report with the POSIX example and KOSAKI-san's
explanation.

http://sourceware.org/bugzilla/show_activity.cgi?id=13690

---
Atsushi Nemoto
Reply | Threaded
Open this post in threaded view
|

Re: pthread_mutex_unlock potentially cause invalid access

Carlos O'Donell-2
On Tue, Feb 14, 2012 at 9:57 AM, Atsushi Nemoto <[hidden email]> wrote:

> On Mon, 13 Feb 2012 10:28:49 -0500, "Carlos O'Donell" <[hidden email]> wrote:
>> Excellent, *this* makes sense and I now agree that there is a problem.
>>
>> Nemoto-san could you please file a bug report and tag it with
>> glibc_2.15 keyword so I can backport this to 2.15?
>
> Yes, I filed a bug report with the POSIX example and KOSAKI-san's
> explanation.
>
> http://sourceware.org/bugzilla/show_activity.cgi?id=13690

Thanks!

 I'm following up in the bug report.

Cheers,
Carlos.