Skip to content

Commit 9552bcd

Browse files
committed
Auto merge of #33861 - Amanieu:lock_elision_fix, r=alexcrichton
Make sure Mutex and RwLock can't be re-locked on the same thread Fixes #33770 r? @alexcrichton
2 parents 95206f4 + fc4b356 commit 9552bcd

File tree

7 files changed

+214
-11
lines changed

7 files changed

+214
-11
lines changed

src/libstd/sync/mutex.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -204,10 +204,14 @@ impl<T> Mutex<T> {
204204
/// Creates a new mutex in an unlocked state ready for use.
205205
#[stable(feature = "rust1", since = "1.0.0")]
206206
pub fn new(t: T) -> Mutex<T> {
207-
Mutex {
207+
let mut m = Mutex {
208208
inner: box StaticMutex::new(),
209209
data: UnsafeCell::new(t),
210+
};
211+
unsafe {
212+
m.inner.lock.init();
210213
}
214+
m
211215
}
212216
}
213217

src/libstd/sys/common/mutex.rs

+6
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ impl Mutex {
2727
/// first used with any of the functions below.
2828
pub const fn new() -> Mutex { Mutex(imp::Mutex::new()) }
2929

30+
/// Prepare the mutex for use.
31+
///
32+
/// This should be called once the mutex is at a stable memory address.
33+
#[inline]
34+
pub unsafe fn init(&mut self) { self.0.init() }
35+
3036
/// Locks the mutex blocking the current thread until it is available.
3137
///
3238
/// Behavior is undefined if the mutex has been moved between this and any

src/libstd/sys/unix/mutex.rs

+33
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,39 @@ impl Mutex {
3030
Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
3131
}
3232
#[inline]
33+
pub unsafe fn init(&mut self) {
34+
// Issue #33770
35+
//
36+
// A pthread mutex initialized with PTHREAD_MUTEX_INITIALIZER will have
37+
// a type of PTHREAD_MUTEX_DEFAULT, which has undefined behavior if you
38+
// try to re-lock it from the same thread when you already hold a lock.
39+
//
40+
// In practice, glibc takes advantage of this undefined behavior to
41+
// implement hardware lock elision, which uses hardware transactional
42+
// memory to avoid acquiring the lock. While a transaction is in
43+
// progress, the lock appears to be unlocked. This isn't a problem for
44+
// other threads since the transactional memory will abort if a conflict
45+
// is detected, however no abort is generated if re-locking from the
46+
// same thread.
47+
//
48+
// Since locking the same mutex twice will result in two aliasing &mut
49+
// references, we instead create the mutex with type
50+
// PTHREAD_MUTEX_NORMAL which is guaranteed to deadlock if we try to
51+
// re-lock it from the same thread, thus avoiding undefined behavior.
52+
//
53+
// We can't do anything for StaticMutex, but that type is deprecated
54+
// anyways.
55+
let mut attr: libc::pthread_mutexattr_t = mem::uninitialized();
56+
let r = libc::pthread_mutexattr_init(&mut attr);
57+
debug_assert_eq!(r, 0);
58+
let r = libc::pthread_mutexattr_settype(&mut attr, libc::PTHREAD_MUTEX_NORMAL);
59+
debug_assert_eq!(r, 0);
60+
let r = libc::pthread_mutex_init(self.inner.get(), &attr);
61+
debug_assert_eq!(r, 0);
62+
let r = libc::pthread_mutexattr_destroy(&mut attr);
63+
debug_assert_eq!(r, 0);
64+
}
65+
#[inline]
3366
pub unsafe fn lock(&self) {
3467
let r = libc::pthread_mutex_lock(self.inner.get());
3568
debug_assert_eq!(r, 0);

src/libstd/sys/unix/rwlock.rs

+67-9
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,24 @@
1010

1111
use libc;
1212
use cell::UnsafeCell;
13+
use sync::atomic::{AtomicUsize, Ordering};
1314

14-
pub struct RWLock { inner: UnsafeCell<libc::pthread_rwlock_t> }
15+
pub struct RWLock {
16+
inner: UnsafeCell<libc::pthread_rwlock_t>,
17+
write_locked: UnsafeCell<bool>,
18+
num_readers: AtomicUsize,
19+
}
1520

1621
unsafe impl Send for RWLock {}
1722
unsafe impl Sync for RWLock {}
1823

1924
impl RWLock {
2025
pub const fn new() -> RWLock {
21-
RWLock { inner: UnsafeCell::new(libc::PTHREAD_RWLOCK_INITIALIZER) }
26+
RWLock {
27+
inner: UnsafeCell::new(libc::PTHREAD_RWLOCK_INITIALIZER),
28+
write_locked: UnsafeCell::new(false),
29+
num_readers: AtomicUsize::new(0),
30+
}
2231
}
2332
#[inline]
2433
pub unsafe fn read(&self) {
@@ -35,37 +44,86 @@ impl RWLock {
3544
//
3645
// We roughly maintain the deadlocking behavior by panicking to ensure
3746
// that this lock acquisition does not succeed.
38-
if r == libc::EDEADLK {
47+
//
48+
// We also check whether there this lock is already write locked. This
49+
// is only possible if it was write locked by the current thread and
50+
// the implementation allows recursive locking. The POSIX standard
51+
// doesn't require recursivly locking a rwlock to deadlock, but we can't
52+
// allow that because it could lead to aliasing issues.
53+
if r == libc::EDEADLK || *self.write_locked.get() {
54+
if r == 0 {
55+
self.raw_unlock();
56+
}
3957
panic!("rwlock read lock would result in deadlock");
4058
} else {
4159
debug_assert_eq!(r, 0);
60+
self.num_readers.fetch_add(1, Ordering::Relaxed);
4261
}
4362
}
4463
#[inline]
4564
pub unsafe fn try_read(&self) -> bool {
46-
libc::pthread_rwlock_tryrdlock(self.inner.get()) == 0
65+
let r = libc::pthread_rwlock_tryrdlock(self.inner.get());
66+
if r == 0 {
67+
if *self.write_locked.get() {
68+
self.raw_unlock();
69+
false
70+
} else {
71+
self.num_readers.fetch_add(1, Ordering::Relaxed);
72+
true
73+
}
74+
} else {
75+
false
76+
}
4777
}
4878
#[inline]
4979
pub unsafe fn write(&self) {
5080
let r = libc::pthread_rwlock_wrlock(self.inner.get());
51-
// see comments above for why we check for EDEADLK
52-
if r == libc::EDEADLK {
81+
// See comments above for why we check for EDEADLK and write_locked. We
82+
// also need to check that num_readers is 0.
83+
if r == libc::EDEADLK || *self.write_locked.get() ||
84+
self.num_readers.load(Ordering::Relaxed) != 0 {
85+
if r == 0 {
86+
self.raw_unlock();
87+
}
5388
panic!("rwlock write lock would result in deadlock");
5489
} else {
5590
debug_assert_eq!(r, 0);
5691
}
92+
*self.write_locked.get() = true;
5793
}
5894
#[inline]
5995
pub unsafe fn try_write(&self) -> bool {
60-
libc::pthread_rwlock_trywrlock(self.inner.get()) == 0
96+
let r = libc::pthread_rwlock_trywrlock(self.inner.get());
97+
if r == 0 {
98+
if *self.write_locked.get() || self.num_readers.load(Ordering::Relaxed) != 0 {
99+
self.raw_unlock();
100+
false
101+
} else {
102+
*self.write_locked.get() = true;
103+
true
104+
}
105+
} else {
106+
false
107+
}
61108
}
62109
#[inline]
63-
pub unsafe fn read_unlock(&self) {
110+
unsafe fn raw_unlock(&self) {
64111
let r = libc::pthread_rwlock_unlock(self.inner.get());
65112
debug_assert_eq!(r, 0);
66113
}
67114
#[inline]
68-
pub unsafe fn write_unlock(&self) { self.read_unlock() }
115+
pub unsafe fn read_unlock(&self) {
116+
debug_assert!(!*self.write_locked.get());
117+
self.num_readers.fetch_sub(1, Ordering::Relaxed);
118+
self.raw_unlock();
119+
}
120+
#[inline]
121+
pub unsafe fn write_unlock(&self) {
122+
debug_assert_eq!(self.num_readers.load(Ordering::Relaxed), 0);
123+
debug_assert!(*self.write_locked.get());
124+
*self.write_locked.get() = false;
125+
self.raw_unlock();
126+
}
69127
#[inline]
70128
pub unsafe fn destroy(&self) {
71129
let r = libc::pthread_rwlock_destroy(self.inner.get());

src/libstd/sys/windows/mutex.rs

+2
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ impl Mutex {
6464
held: UnsafeCell::new(false),
6565
}
6666
}
67+
#[inline]
68+
pub unsafe fn init(&mut self) {}
6769
pub unsafe fn lock(&self) {
6870
match kind() {
6971
Kind::SRWLock => c::AcquireSRWLockExclusive(raw(self)),

src/test/run-pass/issue-33770.rs

+100
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
use std::process::{Command, Stdio};
12+
use std::env;
13+
use std::sync::{Mutex, RwLock};
14+
use std::time::Duration;
15+
use std::thread;
16+
17+
fn test_mutex() {
18+
let m = Mutex::new(0);
19+
let _g = m.lock().unwrap();
20+
let _g2 = m.lock().unwrap();
21+
}
22+
23+
fn test_try_mutex() {
24+
let m = Mutex::new(0);
25+
let _g = m.lock().unwrap();
26+
let _g2 = m.try_lock().unwrap();
27+
}
28+
29+
fn test_rwlock_ww() {
30+
let m = RwLock::new(0);
31+
let _g = m.write().unwrap();
32+
let _g2 = m.write().unwrap();
33+
}
34+
35+
fn test_try_rwlock_ww() {
36+
let m = RwLock::new(0);
37+
let _g = m.write().unwrap();
38+
let _g2 = m.try_write().unwrap();
39+
}
40+
41+
fn test_rwlock_rw() {
42+
let m = RwLock::new(0);
43+
let _g = m.read().unwrap();
44+
let _g2 = m.write().unwrap();
45+
}
46+
47+
fn test_try_rwlock_rw() {
48+
let m = RwLock::new(0);
49+
let _g = m.read().unwrap();
50+
let _g2 = m.try_write().unwrap();
51+
}
52+
53+
fn test_rwlock_wr() {
54+
let m = RwLock::new(0);
55+
let _g = m.write().unwrap();
56+
let _g2 = m.read().unwrap();
57+
}
58+
59+
fn test_try_rwlock_wr() {
60+
let m = RwLock::new(0);
61+
let _g = m.write().unwrap();
62+
let _g2 = m.try_read().unwrap();
63+
}
64+
65+
fn main() {
66+
let args: Vec<String> = env::args().collect();
67+
if args.len() > 1 {
68+
match &*args[1] {
69+
"mutex" => test_mutex(),
70+
"try_mutex" => test_try_mutex(),
71+
"rwlock_ww" => test_rwlock_ww(),
72+
"try_rwlock_ww" => test_try_rwlock_ww(),
73+
"rwlock_rw" => test_rwlock_rw(),
74+
"try_rwlock_rw" => test_try_rwlock_rw(),
75+
"rwlock_wr" => test_rwlock_wr(),
76+
"try_rwlock_wr" => test_try_rwlock_wr(),
77+
_ => unreachable!(),
78+
}
79+
// If we reach this point then the test failed
80+
println!("TEST FAILED: {}", args[1]);
81+
} else {
82+
let mut v = vec![];
83+
v.push(Command::new(&args[0]).arg("mutex").stderr(Stdio::null()).spawn().unwrap());
84+
v.push(Command::new(&args[0]).arg("try_mutex").stderr(Stdio::null()).spawn().unwrap());
85+
v.push(Command::new(&args[0]).arg("rwlock_ww").stderr(Stdio::null()).spawn().unwrap());
86+
v.push(Command::new(&args[0]).arg("try_rwlock_ww").stderr(Stdio::null()).spawn().unwrap());
87+
v.push(Command::new(&args[0]).arg("rwlock_rw").stderr(Stdio::null()).spawn().unwrap());
88+
v.push(Command::new(&args[0]).arg("try_rwlock_rw").stderr(Stdio::null()).spawn().unwrap());
89+
v.push(Command::new(&args[0]).arg("rwlock_wr").stderr(Stdio::null()).spawn().unwrap());
90+
v.push(Command::new(&args[0]).arg("try_rwlock_wr").stderr(Stdio::null()).spawn().unwrap());
91+
92+
thread::sleep(Duration::new(1, 0));
93+
94+
// Make sure all subprocesses either panicked or were killed because they deadlocked
95+
for mut c in v {
96+
c.kill().ok();
97+
assert!(!c.wait().unwrap().success());
98+
}
99+
}
100+
}

0 commit comments

Comments
 (0)