Skip to content

Commit 14ecef6

Browse files
authored
Fix Stacked Borrows UB (and format the code) (zesterer#136)
* Fixed stacked borrows UB * fmt
1 parent 7cd5133 commit 14ecef6

File tree

9 files changed

+258
-164
lines changed

9 files changed

+258
-164
lines changed

src/barrier.rs

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,7 @@ impl<R: RelaxStrategy> Barrier<R> {
115115
// not the leader
116116
let local_gen = lock.generation_id;
117117

118-
while local_gen == lock.generation_id &&
119-
lock.count < self.num_threads {
118+
while local_gen == lock.generation_id && lock.count < self.num_threads {
120119
drop(lock);
121120
R::relax();
122121
lock = self.lock.lock();
@@ -176,7 +175,9 @@ impl BarrierWaitResult {
176175
/// let barrier_wait_result = barrier.wait();
177176
/// println!("{:?}", barrier_wait_result.is_leader());
178177
/// ```
179-
pub fn is_leader(&self) -> bool { self.0 }
178+
pub fn is_leader(&self) -> bool {
179+
self.0
180+
}
180181
}
181182

182183
#[cfg(test)]
@@ -192,12 +193,13 @@ mod tests {
192193
fn use_barrier(n: usize, barrier: Arc<Barrier>) {
193194
let (tx, rx) = channel();
194195

196+
let mut ts = Vec::new();
195197
for _ in 0..n - 1 {
196198
let c = barrier.clone();
197199
let tx = tx.clone();
198-
thread::spawn(move|| {
200+
ts.push(thread::spawn(move || {
199201
tx.send(c.wait().is_leader()).unwrap();
200-
});
202+
}));
201203
}
202204

203205
// At this point, all spawned threads should be blocked,
@@ -217,6 +219,10 @@ mod tests {
217219
}
218220
}
219221
assert!(leader_found);
222+
223+
for t in ts {
224+
t.join().unwrap();
225+
}
220226
}
221227

222228
#[test]

src/lazy.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
//! Implementation adapted from the `SyncLazy` type of the standard library. See:
44
//! <https://doc.rust-lang.org/std/lazy/struct.SyncLazy.html>
55
6-
use core::{cell::Cell, fmt, ops::Deref};
76
use crate::{once::Once, RelaxStrategy, Spin};
7+
use core::{cell::Cell, fmt, ops::Deref};
88

99
/// A value which is initialized on the first access.
1010
///
@@ -45,7 +45,10 @@ pub struct Lazy<T, F = fn() -> T, R = Spin> {
4545

4646
impl<T: fmt::Debug, F, R> fmt::Debug for Lazy<T, F, R> {
4747
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
48-
f.debug_struct("Lazy").field("cell", &self.cell).field("init", &"..").finish()
48+
f.debug_struct("Lazy")
49+
.field("cell", &self.cell)
50+
.field("init", &"..")
51+
.finish()
4952
}
5053
}
5154

@@ -61,7 +64,10 @@ impl<T, F, R> Lazy<T, F, R> {
6164
/// Creates a new lazy value with the given initializing
6265
/// function.
6366
pub const fn new(f: F) -> Self {
64-
Self { cell: Once::new(), init: Cell::new(Some(f)) }
67+
Self {
68+
cell: Once::new(),
69+
init: Cell::new(Some(f)),
70+
}
6571
}
6672
/// Retrieves a mutable pointer to the inner data.
6773
///

src/lib.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
//! - `lock_api` enables support for [`lock_api`](https://crates.io/crates/lock_api)
5555
//!
5656
//! - `ticket_mutex` uses a ticket lock for the implementation of `Mutex`
57-
//!
57+
//!
5858
//! - `fair_mutex` enables a fairer implementation of `Mutex` that uses eventual fairness to avoid
5959
//! starvation
6060
//!
@@ -66,10 +66,10 @@ extern crate core;
6666
#[cfg(feature = "portable_atomic")]
6767
extern crate portable_atomic;
6868

69-
#[cfg(feature = "portable_atomic")]
70-
use portable_atomic as atomic;
7169
#[cfg(not(feature = "portable_atomic"))]
7270
use core::sync::atomic;
71+
#[cfg(feature = "portable_atomic")]
72+
use portable_atomic as atomic;
7373

7474
#[cfg(feature = "barrier")]
7575
#[cfg_attr(docsrs, doc(cfg(feature = "barrier")))]
@@ -83,21 +83,21 @@ pub mod mutex;
8383
#[cfg(feature = "once")]
8484
#[cfg_attr(docsrs, doc(cfg(feature = "once")))]
8585
pub mod once;
86+
pub mod relax;
8687
#[cfg(feature = "rwlock")]
8788
#[cfg_attr(docsrs, doc(cfg(feature = "rwlock")))]
8889
pub mod rwlock;
89-
pub mod relax;
9090

9191
#[cfg(feature = "mutex")]
9292
#[cfg_attr(docsrs, doc(cfg(feature = "mutex")))]
9393
pub use mutex::MutexGuard;
94-
#[cfg(feature = "rwlock")]
95-
#[cfg_attr(docsrs, doc(cfg(feature = "rwlock")))]
96-
pub use rwlock::RwLockReadGuard;
97-
pub use relax::{Spin, RelaxStrategy};
9894
#[cfg(feature = "std")]
9995
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
10096
pub use relax::Yield;
97+
pub use relax::{RelaxStrategy, Spin};
98+
#[cfg(feature = "rwlock")]
99+
#[cfg_attr(docsrs, doc(cfg(feature = "rwlock")))]
100+
pub use rwlock::RwLockReadGuard;
101101

102102
// Avoid confusing inference errors by aliasing away the relax strategy parameter. Users that need to use a different
103103
// relax strategy can do so by accessing the types through their fully-qualified path. This is a little bit horrible
@@ -198,7 +198,7 @@ pub mod lock_api {
198198

199199
/// In the event of an invalid operation, it's best to abort the current process.
200200
#[cfg(feature = "fair_mutex")]
201-
fn abort() -> !{
201+
fn abort() -> ! {
202202
#[cfg(not(feature = "std"))]
203203
{
204204
// Panicking while panicking is defined by Rust to result in an abort.
@@ -218,4 +218,4 @@ fn abort() -> !{
218218
{
219219
std::process::abort();
220220
}
221-
}
221+
}

src/mutex.rs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ pub mod fair;
3434
#[cfg_attr(docsrs, doc(cfg(feature = "fair_mutex")))]
3535
pub use self::fair::{FairMutex, FairMutexGuard, Starvation};
3636

37+
use crate::{RelaxStrategy, Spin};
3738
use core::{
3839
fmt,
3940
ops::{Deref, DerefMut},
4041
};
41-
use crate::{RelaxStrategy, Spin};
4242

4343
#[cfg(all(not(feature = "spin_mutex"), not(feature = "use_ticket_mutex")))]
4444
compile_error!("The `mutex` feature flag was used (perhaps through another feature?) without either `spin_mutex` or `use_ticket_mutex`. One of these is required.");
@@ -85,9 +85,11 @@ type InnerMutexGuard<'a, T> = self::ticket::TicketMutexGuard<'a, T>;
8585
/// // We use a barrier to ensure the readout happens after all writing
8686
/// let barrier = Arc::new(Barrier::new(thread_count + 1));
8787
///
88+
/// # let mut ts = Vec::new();
8889
/// for _ in (0..thread_count) {
8990
/// let my_barrier = barrier.clone();
9091
/// let my_lock = spin_mutex.clone();
92+
/// # let t =
9193
/// std::thread::spawn(move || {
9294
/// let mut guard = my_lock.lock();
9395
/// *guard += 1;
@@ -96,12 +98,17 @@ type InnerMutexGuard<'a, T> = self::ticket::TicketMutexGuard<'a, T>;
9698
/// drop(guard);
9799
/// my_barrier.wait();
98100
/// });
101+
/// # ts.push(t);
99102
/// }
100103
///
101104
/// barrier.wait();
102105
///
103106
/// let answer = { *spin_mutex.lock() };
104107
/// assert_eq!(answer, thread_count);
108+
///
109+
/// # for t in ts {
110+
/// # t.join().unwrap();
111+
/// # }
105112
/// ```
106113
pub struct Mutex<T: ?Sized, R = Spin> {
107114
inner: InnerMutex<T, R>,
@@ -139,7 +146,9 @@ impl<T, R> Mutex<T, R> {
139146
/// ```
140147
#[inline(always)]
141148
pub const fn new(value: T) -> Self {
142-
Self { inner: InnerMutex::new(value) }
149+
Self {
150+
inner: InnerMutex::new(value),
151+
}
143152
}
144153

145154
/// Consumes this [`Mutex`] and unwraps the underlying data.

0 commit comments

Comments
 (0)