pthread_mutex_unlock potentially cause invalid access

[Carlos O'Donell]

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.

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