Skip to content

Commit bcd5f77

Browse files
bors[bot]cynecx
bors[bot]
and
cynecx
authored
Merge #458
458: Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) r=jeehoonkang a=cynecx Several libs in crossbeam contain unsound code (eg. `Block::new`). This PR fixes them and requires a MSRV upgrade to 1.36. Co-authored-by: cynecx <me@cynecx.net>
2 parents 0e91b78 + 8903df8 commit bcd5f77

File tree

14 files changed

+104
-65
lines changed

14 files changed

+104
-65
lines changed

‎crossbeam-channel/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ description = "Multi-producer multi-consumer channels for message passing"
1515
keywords = ["channel", "mpmc", "select", "golang", "message"]
1616
categories = ["algorithms", "concurrency", "data-structures"]
1717

18+
[dependencies]
19+
maybe-uninit = "2.0.0"
20+
1821
[dependencies.crossbeam-utils]
1922
version = "0.7"
2023
path = "../crossbeam-utils"

‎crossbeam-channel/src/flavors/array.rs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ use std::time::Instant;
2222

2323
use crossbeam_utils::{Backoff, CachePadded};
2424

25+
use maybe_uninit::MaybeUninit;
26+
2527
use context::Context;
2628
use err::{RecvTimeoutError, SendTimeoutError, TryRecvError, TrySendError};
2729
use select::{Operation, SelectHandle, Selected, Token};
@@ -33,7 +35,7 @@ struct Slot<T> {
3335
stamp: AtomicUsize,
3436

3537
/// The message in this slot.
36-
msg: UnsafeCell<T>,
38+
msg: UnsafeCell<MaybeUninit<T>>,
3739
}
3840

3941
/// The token type for the array flavor.
@@ -233,7 +235,7 @@ impl<T> Channel<T> {
233235
let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);
234236

235237
// Write the message into the slot and update the stamp.
236-
slot.msg.get().write(msg);
238+
slot.msg.get().write(MaybeUninit::new(msg));
237239
slot.stamp.store(token.array.stamp, Ordering::Release);
238240

239241
// Wake a sleeping receiver.
@@ -323,7 +325,7 @@ impl<T> Channel<T> {
323325
let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);
324326

325327
// Read the message from the slot and update the stamp.
326-
let msg = slot.msg.get().read();
328+
let msg = slot.msg.get().read().assume_init();
327329
slot.stamp.store(token.array.stamp, Ordering::Release);
328330

329331
// Wake a sleeping sender.
@@ -542,7 +544,12 @@ impl<T> Drop for Channel<T> {
542544
};
543545

544546
unsafe {
545-
self.buffer.add(index).drop_in_place();
547+
let p = {
548+
let slot = &mut *self.buffer.add(index);
549+
let msg = &mut *slot.msg.get();
550+
msg.as_mut_ptr()
551+
};
552+
p.drop_in_place();
546553
}
547554
}
548555

‎crossbeam-channel/src/flavors/list.rs

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@
22
33
use std::cell::UnsafeCell;
44
use std::marker::PhantomData;
5-
use std::mem::{self, ManuallyDrop};
65
use std::ptr;
76
use std::sync::atomic::{self, AtomicPtr, AtomicUsize, Ordering};
87
use std::time::Instant;
98

109
use crossbeam_utils::{Backoff, CachePadded};
1110

11+
use maybe_uninit::MaybeUninit;
12+
1213
use context::Context;
1314
use err::{RecvTimeoutError, SendTimeoutError, TryRecvError, TrySendError};
1415
use select::{Operation, SelectHandle, Selected, Token};
@@ -42,7 +43,7 @@ const MARK_BIT: usize = 1;
4243
/// A slot in a block.
4344
struct Slot<T> {
4445
/// The message.
45-
msg: UnsafeCell<ManuallyDrop<T>>,
46+
msg: UnsafeCell<MaybeUninit<T>>,
4647

4748
/// The state of the slot.
4849
state: AtomicUsize,
@@ -72,7 +73,13 @@ struct Block<T> {
7273
impl<T> Block<T> {
7374
/// Creates an empty block.
7475
fn new() -> Block<T> {
75-
unsafe { mem::zeroed() }
76+
// SAFETY: This is safe because:
77+
// [1] `Block::next` (AtomicPtr) may be safely zero initialized.
78+
// [2] `Block::slots` (Array) may be safely zero initialized because of [3, 4].
79+
// [3] `Slot::msg` (UnsafeCell) may be safely zero initialized because it
80+
// holds a MaybeUninit.
81+
// [4] `Slot::state` (AtomicUsize) may be safely zero initialized.
82+
unsafe { MaybeUninit::zeroed().assume_init() }
7683
}
7784

7885
/// Waits until the next pointer is set.
@@ -280,7 +287,7 @@ impl<T> Channel<T> {
280287
let block = token.list.block as *mut Block<T>;
281288
let offset = token.list.offset;
282289
let slot = (*block).slots.get_unchecked(offset);
283-
slot.msg.get().write(ManuallyDrop::new(msg));
290+
slot.msg.get().write(MaybeUninit::new(msg));
284291
slot.state.fetch_or(WRITE, Ordering::Release);
285292

286293
// Wake a sleeping receiver.
@@ -385,8 +392,7 @@ impl<T> Channel<T> {
385392
let offset = token.list.offset;
386393
let slot = (*block).slots.get_unchecked(offset);
387394
slot.wait_write();
388-
let m = slot.msg.get().read();
389-
let msg = ManuallyDrop::into_inner(m);
395+
let msg = slot.msg.get().read().assume_init();
390396

391397
// Destroy the block if we've reached the end, or if another thread wanted to destroy but
392398
// couldn't because we were busy reading from the slot.
@@ -572,7 +578,8 @@ impl<T> Drop for Channel<T> {
572578
if offset < BLOCK_CAP {
573579
// Drop the message in the slot.
574580
let slot = (*block).slots.get_unchecked(offset);
575-
ManuallyDrop::drop(&mut *(*slot).msg.get());
581+
let p = &mut *slot.msg.get();
582+
p.as_mut_ptr().drop_in_place();
576583
} else {
577584
// Deallocate the block and move to the next one.
578585
let next = (*block).next.load(Ordering::Relaxed);

‎crossbeam-channel/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,7 @@
348348
#![warn(missing_debug_implementations)]
349349

350350
extern crate crossbeam_utils;
351+
extern crate maybe_uninit;
351352

352353
mod channel;
353354
mod context;

‎crossbeam-deque/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ description = "Concurrent work-stealing deque"
1515
keywords = ["chase-lev", "lock-free", "scheduler", "scheduling"]
1616
categories = ["algorithms", "concurrency", "data-structures"]
1717

18+
[dependencies]
19+
maybe-uninit = "2.0.0"
20+
1821
[dependencies.crossbeam-epoch]
1922
version = "0.8"
2023
path = "../crossbeam-epoch"

‎crossbeam-deque/src/lib.rs

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -92,19 +92,23 @@
9292
extern crate crossbeam_epoch as epoch;
9393
extern crate crossbeam_utils as utils;
9494

95+
extern crate maybe_uninit;
96+
9597
use std::cell::{Cell, UnsafeCell};
9698
use std::cmp;
9799
use std::fmt;
98100
use std::iter::FromIterator;
99101
use std::marker::PhantomData;
100-
use std::mem::{self, ManuallyDrop};
102+
use std::mem;
101103
use std::ptr;
102104
use std::sync::atomic::{self, AtomicIsize, AtomicPtr, AtomicUsize, Ordering};
103105
use std::sync::Arc;
104106

105107
use epoch::{Atomic, Owned};
106108
use utils::{Backoff, CachePadded};
107109

110+
use maybe_uninit::MaybeUninit;
111+
108112
// Minimum buffer capacity.
109113
const MIN_CAP: usize = 64;
110114
// Maximum number of tasks that can be stolen in `steal_batch()` and `steal_batch_and_pop()`.
@@ -218,7 +222,7 @@ impl<T> Drop for Inner<T> {
218222
// Go through the buffer from front to back and drop all tasks in the queue.
219223
let mut i = f;
220224
while i != b {
221-
ptr::drop_in_place(buffer.deref().at(i));
225+
buffer.deref().at(i).drop_in_place();
222226
i = i.wrapping_add(1);
223227
}
224228

@@ -1140,7 +1144,7 @@ const HAS_NEXT: usize = 1;
11401144
/// A slot in a block.
11411145
struct Slot<T> {
11421146
/// The task.
1143-
task: UnsafeCell<ManuallyDrop<T>>,
1147+
task: UnsafeCell<MaybeUninit<T>>,
11441148

11451149
/// The state of the slot.
11461150
state: AtomicUsize,
@@ -1170,7 +1174,13 @@ struct Block<T> {
11701174
impl<T> Block<T> {
11711175
/// Creates an empty block that starts at `start_index`.
11721176
fn new() -> Block<T> {
1173-
unsafe { mem::zeroed() }
1177+
// SAFETY: This is safe because:
1178+
// [1] `Block::next` (AtomicPtr) may be safely zero initialized.
1179+
// [2] `Block::slots` (Array) may be safely zero initialized because of [3, 4].
1180+
// [3] `Slot::task` (UnsafeCell) may be safely zero initialized because it
1181+
// holds a MaybeUninit.
1182+
// [4] `Slot::state` (AtomicUsize) may be safely zero initialized.
1183+
unsafe { MaybeUninit::zeroed().assume_init() }
11741184
}
11751185

11761186
/// Waits until the next pointer is set.
@@ -1329,7 +1339,7 @@ impl<T> Injector<T> {
13291339

13301340
// Write the task into the slot.
13311341
let slot = (*block).slots.get_unchecked(offset);
1332-
slot.task.get().write(ManuallyDrop::new(task));
1342+
slot.task.get().write(MaybeUninit::new(task));
13331343
slot.state.fetch_or(WRITE, Ordering::Release);
13341344

13351345
return;
@@ -1422,8 +1432,7 @@ impl<T> Injector<T> {
14221432
// Read the task.
14231433
let slot = (*block).slots.get_unchecked(offset);
14241434
slot.wait_write();
1425-
let m = slot.task.get().read();
1426-
let task = ManuallyDrop::into_inner(m);
1435+
let task = slot.task.get().read().assume_init();
14271436

14281437
// Destroy the block if we've reached the end, or if another thread wanted to destroy
14291438
// but couldn't because we were busy reading from the slot.
@@ -1548,8 +1557,7 @@ impl<T> Injector<T> {
15481557
// Read the task.
15491558
let slot = (*block).slots.get_unchecked(offset + i);
15501559
slot.wait_write();
1551-
let m = slot.task.get().read();
1552-
let task = ManuallyDrop::into_inner(m);
1560+
let task = slot.task.get().read().assume_init();
15531561

15541562
// Write it into the destination queue.
15551563
dest_buffer.write(dest_b.wrapping_add(i as isize), task);
@@ -1561,8 +1569,7 @@ impl<T> Injector<T> {
15611569
// Read the task.
15621570
let slot = (*block).slots.get_unchecked(offset + i);
15631571
slot.wait_write();
1564-
let m = slot.task.get().read();
1565-
let task = ManuallyDrop::into_inner(m);
1572+
let task = slot.task.get().read().assume_init();
15661573

15671574
// Write it into the destination queue.
15681575
dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task);
@@ -1704,8 +1711,7 @@ impl<T> Injector<T> {
17041711
// Read the task.
17051712
let slot = (*block).slots.get_unchecked(offset);
17061713
slot.wait_write();
1707-
let m = slot.task.get().read();
1708-
let task = ManuallyDrop::into_inner(m);
1714+
let task = slot.task.get().read().assume_init();
17091715

17101716
match dest.flavor {
17111717
Flavor::Fifo => {
@@ -1714,8 +1720,7 @@ impl<T> Injector<T> {
17141720
// Read the task.
17151721
let slot = (*block).slots.get_unchecked(offset + i + 1);
17161722
slot.wait_write();
1717-
let m = slot.task.get().read();
1718-
let task = ManuallyDrop::into_inner(m);
1723+
let task = slot.task.get().read().assume_init();
17191724

17201725
// Write it into the destination queue.
17211726
dest_buffer.write(dest_b.wrapping_add(i as isize), task);
@@ -1728,8 +1733,7 @@ impl<T> Injector<T> {
17281733
// Read the task.
17291734
let slot = (*block).slots.get_unchecked(offset + i + 1);
17301735
slot.wait_write();
1731-
let m = slot.task.get().read();
1732-
let task = ManuallyDrop::into_inner(m);
1736+
let task = slot.task.get().read().assume_init();
17331737

17341738
// Write it into the destination queue.
17351739
dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task);
@@ -1804,7 +1808,8 @@ impl<T> Drop for Injector<T> {
18041808
if offset < BLOCK_CAP {
18051809
// Drop the task in the slot.
18061810
let slot = (*block).slots.get_unchecked(offset);
1807-
ManuallyDrop::drop(&mut *(*slot).task.get());
1811+
let p = &mut *slot.task.get();
1812+
p.as_mut_ptr().drop_in_place();
18081813
} else {
18091814
// Deallocate the block and move to the next one.
18101815
let next = (*block).next.load(Ordering::Relaxed);

‎crossbeam-epoch/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ sanitize = [] # Makes it more likely to trigger any potential data races.
2424

2525
[dependencies]
2626
cfg-if = "0.1.2"
27+
maybe-uninit = "2.0.0"
2728
memoffset = "0.5"
2829

2930
[dependencies.crossbeam-utils]

‎crossbeam-epoch/src/deferred.rs

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ use core::marker::PhantomData;
44
use core::mem;
55
use core::ptr;
66

7+
use maybe_uninit::MaybeUninit;
8+
79
/// Number of words a piece of `Data` can hold.
810
///
911
/// Three words should be enough for the majority of cases. For example, you can fit inside it the
@@ -36,11 +38,8 @@ impl Deferred {
3638

3739
unsafe {
3840
if size <= mem::size_of::<Data>() && align <= mem::align_of::<Data>() {
39-
// TODO(taiki-e): when the minimum supported Rust version is bumped to 1.36+,
40-
// replace this with `mem::MaybeUninit`.
41-
#[allow(deprecated)]
42-
let mut data: Data = mem::uninitialized();
43-
ptr::write(&mut data as *mut Data as *mut F, f);
41+
let mut data = MaybeUninit::<Data>::uninit();
42+
ptr::write(data.as_mut_ptr() as *mut F, f);
4443

4544
unsafe fn call<F: FnOnce()>(raw: *mut u8) {
4645
let f: F = ptr::read(raw as *mut F);
@@ -49,16 +48,13 @@ impl Deferred {
4948

5049
Deferred {
5150
call: call::<F>,
52-
data,
51+
data: data.assume_init(),
5352
_marker: PhantomData,
5453
}
5554
} else {
5655
let b: Box<F> = Box::new(f);
57-
// TODO(taiki-e): when the minimum supported Rust version is bumped to 1.36+,
58-
// replace this with `mem::MaybeUninit`.
59-
#[allow(deprecated)]
60-
let mut data: Data = mem::uninitialized();
61-
ptr::write(&mut data as *mut Data as *mut Box<F>, b);
56+
let mut data = MaybeUninit::<Data>::uninit();
57+
ptr::write(data.as_mut_ptr() as *mut Box<F>, b);
6258

6359
unsafe fn call<F: FnOnce()>(raw: *mut u8) {
6460
let b: Box<F> = ptr::read(raw as *mut Box<F>);
@@ -67,7 +63,7 @@ impl Deferred {
6763

6864
Deferred {
6965
call: call::<F>,
70-
data,
66+
data: data.assume_init(),
7167
_marker: PhantomData,
7268
}
7369
}

‎crossbeam-epoch/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ extern crate cfg_if;
6464
#[cfg(feature = "std")]
6565
extern crate core;
6666

67+
extern crate maybe_uninit;
68+
6769
cfg_if! {
6870
if #[cfg(feature = "alloc")] {
6971
extern crate alloc;

0 commit comments

Comments
 (0)