Re: Thread-unsafe access to lock->owner in PR_Lock

Hi Alexander,

----- Original Message -----
> From: "Alexander Potapenko" <[email protected]>
> To: [email protected]
> Sent: Wednesday, December 18, 2013 3:46:30 AM
> Subject: Thread-unsafe access to lock->owner in PR_Lock
> 
> Hi NSPR developers,
> 
> while running Chromium tests under ThreadSanitizer
> (https://code.google.com/p/data-race-test/) we've encountered a report about
> a data race between PR_Lock and PR_Unlock: http://crbug.com/328521
> 
> It turns out that the lock->owner variable is being read by PR_Lock at the
> same time other threads read or write it in PR_Lock or PR_Unlock:
> 
> 189 PR_IMPLEMENT(void) PR_Lock(PRLock *lock)
> 190 {
> 191     PRThread *me = _PR_MD_CURRENT_THREAD();
> 192     PRIntn is;
> 193     PRThread *t;
> 194     PRCList *q;
> 195
> 196     PR_ASSERT(me != suspendAllThread);
> 197     PR_ASSERT(!(me->flags & _PR_IDLE_THREAD));
> 198     PR_ASSERT(lock != NULL);
> 199 #ifdef _PR_GLOBAL_THREADS_ONLY
> 200     PR_ASSERT(lock->owner != me);
> 201     _PR_MD_LOCK(&lock->ilock);
> 202     lock->owner = me;
> 203     return;
> 204 #else  /* _PR_GLOBAL_THREADS_ONLY */
> 
> (code from nspr-4.9.5/mozilla/nsprpub/pr/src/threads/combined/prulock.c in
> Ubuntu Precise)
> 
> In the case the optimizer is conservative enough nothing will break (during
> the race lock->owner will be either another thread or NULL, but not |me|),
> but this is undefined behavior in C++11 and can possibly break in newer
> compilers.
> 
> Is there a reason to do this check before the lock is taken? I'm guessing
> it's there to avoid reentrancy, so it should be fine to swap lines 200 and
> 201.

If my understanding is correct, if you swap the 2 lines, then you may deadlock yourself rather than see the assert.

http://mozilla.6506.n7.nabble.com/Are-PR-Locks-thread-re-entrant-td244962.html

Dave Hylands