Skip to content

Commit 6f6336b

Browse files
committed
Split sys_common::Mutex in StaticMutex and MovableMutex.
The (unsafe) Mutex from sys_common had a rather complicated interface. You were supposed to call init() manually, unless you could guarantee it was neither moved nor used reentrantly. Calling `destroy()` was also optional, although it was unclear if 1) resources might be leaked or not, and 2) if destroy() should only be called when `init()` was called. This allowed for a number of interesting (confusing?) different ways to use this Mutex, all captured in a single type. In practice, this type was only ever used in two ways: 1. As a static variable. In this case, neither init() nor destroy() are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using the LockGuard, never with raw_lock. 2. As a Boxed variable. In this case, both init() and destroy() are called, it will be moved and possibly used reentrantly. No other combinations are used anywhere in `std`. This change simplifies things by splitting this Mutex type into two types matching the two use cases: StaticMutex and MovableMutex. The interface of both new types is now both safer and simpler. The first one does not call nor expose init/destroy, and the second one calls those automatically in its new() and Drop functions. Also, the locking functions of MovableMutex are no longer unsafe.
1 parent c9e5e6a commit 6f6336b

File tree

13 files changed

+100
-121
lines changed

13 files changed

+100
-121
lines changed

library/std/src/sync/condvar.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -553,8 +553,8 @@ impl Condvar {
553553
unsafe { self.inner.notify_all() }
554554
}
555555

556-
fn verify(&self, mutex: &sys_mutex::Mutex) {
557-
let addr = mutex as *const _ as usize;
556+
fn verify(&self, mutex: &sys_mutex::MovableMutex) {
557+
let addr = mutex.raw() as *const _ as usize;
558558
match self.mutex.compare_and_swap(0, addr, Ordering::SeqCst) {
559559
// If we got out 0, then we have successfully bound the mutex to
560560
// this cvar.

library/std/src/sync/mutex.rs

+4-26
Original file line numberDiff line numberDiff line change
@@ -166,12 +166,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult};
166166
#[stable(feature = "rust1", since = "1.0.0")]
167167
#[cfg_attr(not(test), rustc_diagnostic_item = "mutex_type")]
168168
pub struct Mutex<T: ?Sized> {
169-
// Note that this mutex is in a *box*, not inlined into the struct itself.
170-
// Once a native mutex has been used once, its address can never change (it
171-
// can't be moved). This mutex type can be safely moved at any time, so to
172-
// ensure that the native mutex is used correctly we box the inner mutex to
173-
// give it a constant address.
174-
inner: Box<sys::Mutex>,
169+
inner: sys::MovableMutex,
175170
poison: poison::Flag,
176171
data: UnsafeCell<T>,
177172
}
@@ -218,15 +213,11 @@ impl<T> Mutex<T> {
218213
/// ```
219214
#[stable(feature = "rust1", since = "1.0.0")]
220215
pub fn new(t: T) -> Mutex<T> {
221-
let mut m = Mutex {
222-
inner: box sys::Mutex::new(),
216+
Mutex {
217+
inner: sys::MovableMutex::new(),
223218
poison: poison::Flag::new(),
224219
data: UnsafeCell::new(t),
225-
};
226-
unsafe {
227-
m.inner.init();
228220
}
229-
m
230221
}
231222
}
232223

@@ -378,7 +369,6 @@ impl<T: ?Sized> Mutex<T> {
378369
(ptr::read(inner), ptr::read(poison), ptr::read(data))
379370
};
380371
mem::forget(self);
381-
inner.destroy(); // Keep in sync with the `Drop` impl.
382372
drop(inner);
383373

384374
poison::map_result(poison.borrow(), |_| data.into_inner())
@@ -411,18 +401,6 @@ impl<T: ?Sized> Mutex<T> {
411401
}
412402
}
413403

414-
#[stable(feature = "rust1", since = "1.0.0")]
415-
unsafe impl<#[may_dangle] T: ?Sized> Drop for Mutex<T> {
416-
fn drop(&mut self) {
417-
// This is actually safe b/c we know that there is no further usage of
418-
// this mutex (it's up to the user to arrange for a mutex to get
419-
// dropped, that's not our job)
420-
//
421-
// IMPORTANT: This code must be kept in sync with `Mutex::into_inner`.
422-
unsafe { self.inner.destroy() }
423-
}
424-
}
425-
426404
#[stable(feature = "mutex_from", since = "1.24.0")]
427405
impl<T> From<T> for Mutex<T> {
428406
/// Creates a new mutex in an unlocked state ready for use.
@@ -509,7 +487,7 @@ impl<T: ?Sized + fmt::Display> fmt::Display for MutexGuard<'_, T> {
509487
}
510488
}
511489

512-
pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::Mutex {
490+
pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::MovableMutex {
513491
&guard.lock.inner
514492
}
515493

library/std/src/sys/hermit/args.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ mod imp {
5757
use crate::ptr;
5858
use crate::sys_common::os_str_bytes::*;
5959

60-
use crate::sys_common::mutex::Mutex;
60+
use crate::sys_common::mutex::StaticMutex;
6161

6262
static mut ARGC: isize = 0;
6363
static mut ARGV: *const *const u8 = ptr::null();
64-
static LOCK: Mutex = Mutex::new();
64+
static LOCK: StaticMutex = StaticMutex::new();
6565

6666
pub unsafe fn init(argc: isize, argv: *const *const u8) {
6767
let _guard = LOCK.lock();

library/std/src/sys/unix/args.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ mod imp {
8080
use crate::ptr;
8181
use crate::sync::atomic::{AtomicIsize, AtomicPtr, Ordering};
8282

83-
use crate::sys_common::mutex::Mutex;
83+
use crate::sys_common::mutex::StaticMutex;
8484

8585
static ARGC: AtomicIsize = AtomicIsize::new(0);
8686
static ARGV: AtomicPtr<*const u8> = AtomicPtr::new(ptr::null_mut());
8787
// We never call `ENV_LOCK.init()`, so it is UB to attempt to
8888
// acquire this mutex reentrantly!
89-
static LOCK: Mutex = Mutex::new();
89+
static LOCK: StaticMutex = StaticMutex::new();
9090

9191
unsafe fn really_init(argc: isize, argv: *const *const u8) {
9292
let _guard = LOCK.lock();

library/std/src/sys/unix/os.rs

+4-5
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use crate::slice;
2121
use crate::str;
2222
use crate::sys::cvt;
2323
use crate::sys::fd;
24-
use crate::sys_common::mutex::{Mutex, MutexGuard};
24+
use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard};
2525
use crate::vec;
2626

2727
use libc::{c_char, c_int, c_void};
@@ -470,10 +470,9 @@ pub unsafe fn environ() -> *mut *const *const c_char {
470470
&mut environ
471471
}
472472

473-
pub unsafe fn env_lock() -> MutexGuard<'static> {
474-
// We never call `ENV_LOCK.init()`, so it is UB to attempt to
475-
// acquire this mutex reentrantly!
476-
static ENV_LOCK: Mutex = Mutex::new();
473+
pub unsafe fn env_lock() -> StaticMutexGuard<'static> {
474+
// It is UB to attempt to acquire this mutex reentrantly!
475+
static ENV_LOCK: StaticMutex = StaticMutex::new();
477476
ENV_LOCK.lock()
478477
}
479478

library/std/src/sys/vxworks/args.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ mod imp {
5757
use crate::marker::PhantomData;
5858
use crate::ptr;
5959

60-
use crate::sys_common::mutex::Mutex;
60+
use crate::sys_common::mutex::StaticMutex;
6161

6262
static mut ARGC: isize = 0;
6363
static mut ARGV: *const *const u8 = ptr::null();
64-
static LOCK: Mutex = Mutex::new();
64+
static LOCK: StaticMutex = StaticMutex::new();
6565

6666
pub unsafe fn init(argc: isize, argv: *const *const u8) {
6767
let _guard = LOCK.lock();

library/std/src/sys/vxworks/os.rs

+4-5
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use crate::path::{self, Path, PathBuf};
1010
use crate::slice;
1111
use crate::str;
1212
use crate::sys::cvt;
13-
use crate::sys_common::mutex::{Mutex, MutexGuard};
13+
use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard};
1414
use libc::{self, c_char /*,c_void */, c_int};
1515
/*use sys::fd; this one is probably important */
1616
use crate::vec;
@@ -212,10 +212,9 @@ pub unsafe fn environ() -> *mut *const *const c_char {
212212
&mut environ
213213
}
214214

215-
pub unsafe fn env_lock() -> MutexGuard<'static> {
216-
// We never call `ENV_LOCK.init()`, so it is UB to attempt to
217-
// acquire this mutex reentrantly!
218-
static ENV_LOCK: Mutex = Mutex::new();
215+
pub unsafe fn env_lock() -> StaticMutexGuard<'static> {
216+
// It is UB to attempt to acquire this mutex reentrantly!
217+
static ENV_LOCK: StaticMutex = StaticMutex::new();
219218
ENV_LOCK.lock()
220219
}
221220

library/std/src/sys_common/at_exit_imp.rs

+3-4
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,16 @@
44
55
use crate::mem;
66
use crate::ptr;
7-
use crate::sys_common::mutex::Mutex;
7+
use crate::sys_common::mutex::StaticMutex;
88

99
type Queue = Vec<Box<dyn FnOnce()>>;
1010

1111
// NB these are specifically not types from `std::sync` as they currently rely
1212
// on poisoning and this module needs to operate at a lower level than requiring
1313
// the thread infrastructure to be in place (useful on the borders of
1414
// initialization/destruction).
15-
// We never call `LOCK.init()`, so it is UB to attempt to
16-
// acquire this mutex reentrantly!
17-
static LOCK: Mutex = Mutex::new();
15+
// It is UB to attempt to acquire this mutex reentrantly!
16+
static LOCK: StaticMutex = StaticMutex::new();
1817
static mut QUEUE: *mut Queue = ptr::null_mut();
1918

2019
const DONE: *mut Queue = 1_usize as *mut _;

library/std/src/sys_common/condvar.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::sys::condvar as imp;
2-
use crate::sys_common::mutex::{self, Mutex};
2+
use crate::sys_common::mutex::MovableMutex;
33
use crate::time::Duration;
44

55
/// An OS-based condition variable.
@@ -46,8 +46,8 @@ impl Condvar {
4646
/// Behavior is also undefined if more than one mutex is used concurrently
4747
/// on this condition variable.
4848
#[inline]
49-
pub unsafe fn wait(&self, mutex: &Mutex) {
50-
self.0.wait(mutex::raw(mutex))
49+
pub unsafe fn wait(&self, mutex: &MovableMutex) {
50+
self.0.wait(mutex.raw())
5151
}
5252

5353
/// Waits for a signal on the specified mutex with a timeout duration
@@ -57,8 +57,8 @@ impl Condvar {
5757
/// Behavior is also undefined if more than one mutex is used concurrently
5858
/// on this condition variable.
5959
#[inline]
60-
pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
61-
self.0.wait_timeout(mutex::raw(mutex), dur)
60+
pub unsafe fn wait_timeout(&self, mutex: &MovableMutex, dur: Duration) -> bool {
61+
self.0.wait_timeout(mutex.raw(), dur)
6262
}
6363

6464
/// Deallocates all resources associated with this condition variable.

library/std/src/sys_common/mutex.rs

+66-61
Original file line numberDiff line numberDiff line change
@@ -1,101 +1,106 @@
11
use crate::sys::mutex as imp;
22

3-
/// An OS-based mutual exclusion lock.
3+
/// An OS-based mutual exclusion lock, meant for use in static variables.
4+
///
5+
/// This mutex has a const constructor ([`StaticMutex::new`]), does not
6+
/// implement `Drop` to cleanup resources, and causes UB when moved or used
7+
/// reentrantly.
8+
///
9+
/// This mutex does not implement poisoning.
410
///
5-
/// This is the thinnest cross-platform wrapper around OS mutexes. All usage of
6-
/// this mutex is unsafe and it is recommended to instead use the safe wrapper
7-
/// at the top level of the crate instead of this type.
8-
pub struct Mutex(imp::Mutex);
11+
/// This is a wrapper around `imp::Mutex` that does *not* call `init()` and
12+
/// `destroy()`.
13+
pub struct StaticMutex(imp::Mutex);
914

10-
unsafe impl Sync for Mutex {}
15+
unsafe impl Sync for StaticMutex {}
1116

12-
impl Mutex {
17+
impl StaticMutex {
1318
/// Creates a new mutex for use.
1419
///
1520
/// Behavior is undefined if the mutex is moved after it is
1621
/// first used with any of the functions below.
17-
/// Also, until `init` is called, behavior is undefined if this
18-
/// mutex is ever used reentrantly, i.e., `raw_lock` or `try_lock`
19-
/// are called by the thread currently holding the lock.
22+
/// Also, the behavior is undefined if this mutex is ever used reentrantly,
23+
/// i.e., `lock` is called by the thread currently holding the lock.
2024
#[rustc_const_stable(feature = "const_sys_mutex_new", since = "1.0.0")]
21-
pub const fn new() -> Mutex {
22-
Mutex(imp::Mutex::new())
25+
pub const fn new() -> Self {
26+
Self(imp::Mutex::new())
2327
}
2428

25-
/// Prepare the mutex for use.
29+
/// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
30+
/// will be unlocked.
2631
///
27-
/// This should be called once the mutex is at a stable memory address.
28-
/// If called, this must be the very first thing that happens to the mutex.
29-
/// Calling it in parallel with or after any operation (including another
30-
/// `init()`) is undefined behavior.
32+
/// It is undefined behaviour to call this function while locked, or if the
33+
/// mutex has been moved since the last time this was called.
3134
#[inline]
32-
pub unsafe fn init(&mut self) {
33-
self.0.init()
35+
pub unsafe fn lock(&self) -> StaticMutexGuard<'_> {
36+
self.0.lock();
37+
StaticMutexGuard(&self.0)
3438
}
39+
}
3540

36-
/// Locks the mutex blocking the current thread until it is available.
37-
///
38-
/// Behavior is undefined if the mutex has been moved between this and any
39-
/// previous function call.
41+
#[must_use]
42+
pub struct StaticMutexGuard<'a>(&'a imp::Mutex);
43+
44+
impl Drop for StaticMutexGuard<'_> {
4045
#[inline]
41-
pub unsafe fn raw_lock(&self) {
42-
self.0.lock()
46+
fn drop(&mut self) {
47+
unsafe {
48+
self.0.unlock();
49+
}
4350
}
51+
}
4452

45-
/// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
46-
/// will be unlocked.
53+
/// An OS-based mutual exclusion lock.
54+
///
55+
/// This mutex does *not* have a const constructor, cleans up its resources in
56+
/// its `Drop` implementation, may safely be moved (when not borrowed), and
57+
/// does not cause UB when used reentrantly.
58+
///
59+
/// This mutex does not implement poisoning.
60+
///
61+
/// This is a wrapper around `Box<imp::Mutex>`, to allow the object to be moved
62+
/// without moving the raw mutex.
63+
pub struct MovableMutex(Box<imp::Mutex>);
64+
65+
unsafe impl Sync for MovableMutex {}
66+
67+
impl MovableMutex {
68+
/// Creates a new mutex.
69+
pub fn new() -> Self {
70+
let mut mutex = box imp::Mutex::new();
71+
unsafe { mutex.init() };
72+
Self(mutex)
73+
}
74+
75+
pub(crate) fn raw(&self) -> &imp::Mutex {
76+
&self.0
77+
}
78+
79+
/// Locks the mutex blocking the current thread until it is available.
4780
#[inline]
48-
pub unsafe fn lock(&self) -> MutexGuard<'_> {
49-
self.raw_lock();
50-
MutexGuard(&self.0)
81+
pub fn raw_lock(&self) {
82+
unsafe { self.0.lock() }
5183
}
5284

5385
/// Attempts to lock the mutex without blocking, returning whether it was
5486
/// successfully acquired or not.
55-
///
56-
/// Behavior is undefined if the mutex has been moved between this and any
57-
/// previous function call.
5887
#[inline]
59-
pub unsafe fn try_lock(&self) -> bool {
60-
self.0.try_lock()
88+
pub fn try_lock(&self) -> bool {
89+
unsafe { self.0.try_lock() }
6190
}
6291

6392
/// Unlocks the mutex.
6493
///
6594
/// Behavior is undefined if the current thread does not actually hold the
6695
/// mutex.
67-
///
68-
/// Consider switching from the pair of raw_lock() and raw_unlock() to
69-
/// lock() whenever possible.
7096
#[inline]
7197
pub unsafe fn raw_unlock(&self) {
7298
self.0.unlock()
7399
}
74-
75-
/// Deallocates all resources associated with this mutex.
76-
///
77-
/// Behavior is undefined if there are current or will be future users of
78-
/// this mutex.
79-
#[inline]
80-
pub unsafe fn destroy(&self) {
81-
self.0.destroy()
82-
}
83100
}
84101

85-
// not meant to be exported to the outside world, just the containing module
86-
pub fn raw(mutex: &Mutex) -> &imp::Mutex {
87-
&mutex.0
88-
}
89-
90-
#[must_use]
91-
/// A simple RAII utility for the above Mutex without the poisoning semantics.
92-
pub struct MutexGuard<'a>(&'a imp::Mutex);
93-
94-
impl Drop for MutexGuard<'_> {
95-
#[inline]
102+
impl Drop for MovableMutex {
96103
fn drop(&mut self) {
97-
unsafe {
98-
self.0.unlock();
99-
}
104+
unsafe { self.0.destroy() };
100105
}
101106
}

0 commit comments

Comments
 (0)