Glibc lock elision allows a value to be locked twice

[Amanieu]

When running on a processor which support Intel's Restricted Transactional Memory, glibc will not write to a lock (making it appear to be unlocked) and instead begin a transaction. The transaction will catch any conflicts from other threads and aborts, but this doesn't happen when re-locking from the same thread. Instead the lock gets acquired twice, which results in two mutable references to the same value.

Example code:

use std::sync::{Mutex, RwLock};

fn test_mutex() -> i32 {
    let m = Mutex::new(0);
    let mut g = m.lock().unwrap();
    let g2 = m.lock().unwrap();
    *g = 1;
    *g2
}

fn test_rwlock() -> i32 {
    let m = RwLock::new(0);
    let mut g = m.write().unwrap();
    let g2 = m.write().unwrap();
    *g = 1;
    *g2
}

fn main() {
    println!("{}", test_mutex());
    println!("{}", test_rwlock());
}

Output:

$ ./test
1
1

For Mutex, this can be solved by creating the mutex with pthread_mutexattr_settype(PTHREAD_MUTEX_NORMAL). However there is no way to disable this behavior for pthread rwlocks.

[alexcrichton]

This is... quite surprising! Why is this not a bug in pthread mutexes? Is it unspecified if a lock is reentrant by default?

[Amanieu]

According to the pthread_mutex_lock man page, re-locking from the same thread is undefined behavior for PTHREAD_MUTEX_DEFAULT. However it is defined to deadlock for PTHREAD_MUTEX_NORMAL.

[birkenfeld]

@Amanieu at least on my system with glibc 2.23, PTHREAD_MUTEX_DEFAULT and PTHREAD_MUTEX_NORMAL are both 0. Do you need a special version of glibc for this "feature"?

[Amanieu]

@birkenfeld Those constants are the same for me as well, however any call to pthread_mutexattr_settype with type 0 will disable elision.

[alexcrichton]

Discussed during libs triage yesterday, definitely something we should fix!

[alexcrichton]

Digging into this, we may get lucky and not have to worry about this for rwlocks. There's an interesting article on merging glibc lock elision in 2013 which states:

But what is not clear from the specification is whether or not the same behavior is required for rwlocks. Carlos O'Donell has contacted the Austin Group to ask for clarification; if the official answer is that rwlocks are required to deadlock on re-locks, then the rwlock patches for glibc will not be merged as-is.

And indeed older standards do indeed indicate that wrlock and rdlock are undefined if a write lock is previously held.

A more recent revision, however, has wrlock and rdlock descriptions which tone down wording along the lines of:

The calling thread may deadlock if at the time the call is made it holds the read-write lock (whether a read or write lock).

or

The calling thread may deadlock if at the time the call is made it holds a write lock.

Also note that the most recent publication for mutex lock acquisition does indeed indicate that for a default mutex recursive locking is undefined behavior.

---

A strict interpretation of all this to me would indicate that we could fix this by:

1. Initializing all std::sync::Mutex types with PTHREAD_MUTEX_NORMAL. We can't fix StaticMutex unfortunately but that's a deprecated type now anyway so it should be fine.
2. Rely on rwlock behavior not being undefined, and instead of a flag "is_write_locked". If a read or write lock succeeds and that flag is set, the thread unlocks and then panics.
3. Successful write locks on an rwlock with the flag to false will then set the flag to true on acquisition and false on release.

The change to mutexes should avoid undefined behavior entirely, and it seems like the rwlock behavior isn't undefined so we can go ahead and do the operation and then check after-the-fact. How's that sound to others?

[Amanieu]

Apparently it's more complicated, this test is failing (not deadlocking):

fn test_rwlock_rw() {
    let m = RwLock::new(0);
    let _g = m.read().unwrap();
    let _g2 = m.write().unwrap();
}

It seems that we need to hold a counter of all reader threads... this is going to be a pain to implement...

[arielb1]

We basically need to embed a RefCell within the lock.

[Amanieu]

@arielb1 Except the read counter needs to be atomic: 8999243

[MagaTailor]

I'm unable to build libstd on an ARM Linux, glibc 2.19 system any more:

src/libstd/sys/unix/mutex.rs:58:60: 58:86 error: unresolved name `libc::PTHREAD_MUTEX_NORMAL` [E0425]
src/libstd/sys/unix/mutex.rs:58         let r = libc::pthread_mutexattr_settype(&mut attr, libc::PTHREAD_MUTEX_NORMAL);
                                                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~

pthread.h contains the following:

/* Mutex types.  */
enum
{
  PTHREAD_MUTEX_TIMED_NP,
  PTHREAD_MUTEX_RECURSIVE_NP,
  PTHREAD_MUTEX_ERRORCHECK_NP,
  PTHREAD_MUTEX_ADAPTIVE_NP
#if defined __USE_UNIX98 || defined __USE_XOPEN2K8
  ,
  PTHREAD_MUTEX_NORMAL = PTHREAD_MUTEX_TIMED_NP,
  PTHREAD_MUTEX_RECURSIVE = PTHREAD_MUTEX_RECURSIVE_NP,
  PTHREAD_MUTEX_ERRORCHECK = PTHREAD_MUTEX_ERRORCHECK_NP,
  PTHREAD_MUTEX_DEFAULT = PTHREAD_MUTEX_NORMAL
#endif
#ifdef __USE_GNU
  /* For compatibility.  */
  , PTHREAD_MUTEX_FAST_NP = PTHREAD_MUTEX_TIMED_NP
#endif
};

[Amanieu]

I think a new version of the libc crate needs to be published on cargo.

cc @alexcrichton

[alexcrichton]

@petevine how are you hitting that error? This passed our CI which means that the constant should be defined for ARM Linux (that's something we gate on). If this is using the libc crate on crates.io how is that coming into play? The libstd in this repo should be using a pinned rev of libc, if you're using something else then that's not guaranteed to work.

[MagaTailor]

I did a git pull yesterday and started a Makefiles based build from scratch.

[Amanieu]

Did you run git submodule update?

[MagaTailor]

No, cause that's done automatically by the script, right? At least it has worked flawlessly up to now it seems.

[MagaTailor]

@alexcrichton I've just done another git pull which immediately pulled from the updated llvm and then proceeded to build libstd just fine. I think these events must be related, and the submodules weren't updated for some reason before.