This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: pthread_mutex_unlock potentially cause invalid access
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.