pthread_mutex_unlock potentially cause invalid access

[Carlos O'Donell]

On Mon, Feb 13, 2012 at 10:02 AM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> 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.