Mutex and RwLock are unsound in presence of discriminant elision

Neither Mutex type nor RwLock type inhibit layout optimization and so either of
them might contain a niche. When a niche is used to perform a discriminant
elision extracting the discriminant of the outer type will inadvertently load
the content of Mutex / RwLock without proper synchronization.

Can you show an example where this is happening? I can't come up with an example where the niche inside the Mutex is ever used where there even is an initialized Mutex at all.

For example following code contains a data race on x86_64-unknown-linux-gnu, between store in thread1 and load in thread2.

use std::mem::size_of;
use std::sync::Arc;
use std::sync::Mutex;

#[inline(never)]
#[no_mangle]
pub fn thread1(v: &mut bool) {
    *v = true;
}

#[inline(never)]
#[no_mangle]
pub fn thread2(v: &Option<Mutex<bool>>) -> usize {
    match *v {
        None => 0,
        Some(_) => 1,
    }
}

fn main() {
    // Verify that discriminant elision takes place.
    assert_eq!(
        size_of::<Mutex<bool>>(),
        size_of::<Option<Mutex<bool>>>(),
    );

    let a = Arc::new(Some(Mutex::new(false)));
    let b = a.clone();

    let t = std::thread::spawn(move || {
        thread1(&mut (*a).as_ref().unwrap().lock().unwrap())
    });

    println!("{}", thread2(&b));
    t.join().unwrap();
}

The Option<Mutex<T>> combination is not uncommon. It happens when a mutex is placed inside a lazy static for example.

Note that there's another niche in Mutex, the Box<sys::Mutex> field, which also enables e.g. Option<Mutex<i32>> to have the same size as Mutex<i32>. But in this case, indeed the data: UnsafeCell<T> field is used for storing the discriminant (presumably because it has a bigger niche?). This can be confirmed by looking at the assembly code generated for this playground and noticing that the same offset is used for locating the data in the mutex as for locating the discriminant.

A simple fix would be to change the UnsafeCell<T> field to UnsafeCell<MaybeUninit<T>>. In reality this field is always initialized, but MaybeUninit<T> has no niche regardless of T, so it would stop the layout optimizations.

I'd say the problem is rather with UnsafeCell itself. match shouldn't look for the tag through the boundary of an UnsafeCell at all, as accessing its value is inherently unsafe and can only be safe if you know the invariants of the safe abstraction on top, which match in general can not know. So UnsafeCell, like MaybeUninit shouldn't allow outside tags to use niches of its T.

I'd say the problem is rather with UnsafeCell itself. match shouldn't look for the tag through the boundary of an UnsafeCell at all, as accessing its value is inherently unsafe and can only be safe if you know the invariants of the safe abstraction on top, which match in general can not know.

I tend to agree that the UnsafeCell<T> exported from std should not allow uses of niches within T

However, I also tend to think that we should have Cell<T> continue to allow use of the niches in its T

This then raises the immediate question of whether we will need to make a new variant of UnsafeCell<T>, e.g. UnsafeCellWithExposedNiches<T> (deliberately ugly name there), that Cell can use...

Cell<T> having the same niche as T should be sound and sensible, but I don't expect it'll actually matter much whether it's done or not. In my experience, type combinations that would benefit from it are pretty rare: generally cells are not (directly) placed in enums, as they are rarely passed around by value in general. If a field with interior mutability is optional, then Cell<Option<T>> is easier to work with and strictly more general. Types containing cells being wrapped in enums is slightly more likely (e.g. with a fallible constructor (fn new(...) -> Result<Self, E>), but I still don't recall ever seeing it in practice.

On its own that isn't a reason to leave the optimization opporturnity on the table, it would still be nice to have if only for "peace of mind". But I don't think hacks such as adding a second variant of UnsafeCell are worthwhile. I'd rather wait and hope that Rust gains a more general and first-class (than rustc_layout_scalar_valid_range_{start,end}) way of declaring niches that allows Cell<T> to inherit the niche of T without such hacks.

But I don't think hacks such as adding a second variant of UnsafeCell are worthwhile. I'd rather wait and hope that Rust gains a more general and first-class (than rustc_layout_scalar_valid_range_{start,end}) way of declaring niches that allows Cell to inherit the niche of T without such hacks.

I think it would be useful to have a perma-unstable UnsafeCellWithExposedNiches type that's only used within libstd. If we can avoid penalizing user code (even if it's rare), I think we should do so. UnsafeCellWithExposedNiches could then be removed whenever a more general rustc_layout_scalar_valid_range_ is developed.

It seems to me that this bug also happens in parking_lot and atomic types, because they don't put MaybeUninit in the UnsafeCell. Is it correct?

Atomics have no niches, but Sync types that use UnsafeCell with user provided type are likely to be affected, including parking_lot.

In terms of naming one might signify !Sync aspect and name variant that participates in layout optimization as UnsafeUnsyncCell.

Ah, good catch! This is very closely related to rust-lang/unsafe-code-guidelines#204, I think. Basically, Rust's notion of "disjointness" says that the discriminant is disjoint from the actual enum data, but with niche optimizations that is not actually the case.

@rpjohnst

@rpjohnst I think it's fine because it doesn't let you make any assumptions, the Cell projections only go through product types.

Also, creating a Cell where there was none requires a mutable reference, which I believe is wrong enough to account for all possible effects.

Rollup merge of rust-lang#68491 - pnkfelix:hide-niches-under-unsafe-c…

Notably, T and Cell<T> are still the same -- just when wrapped in some other type, they might differ (and specifically not when wrapped in a product type). So I do not see how there can be a problem here. But I might be missing something.

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rpjohnst

@rpjohnst a good catch for sure, definitely something to think about.

(assigning self, since I authored PR #68491)

Bumped patch release,updated layout for Cell and UnsafeCell due to ru…

Unsafecell fix (#32)

Navigation Menu

Repository navigation

Mutex and RwLock are unsound in presence of discriminant elision #68206

28 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Mutex and RwLock are unsound in presence of discriminant elision #68206

Description

Activity

CryZe commented on Jan 14, 2020

tmiasko commented on Jan 14, 2020

hanna-kruppe commented on Jan 14, 2020

CryZe commented on Jan 14, 2020

pnkfelix commented on Jan 14, 2020

hanna-kruppe commented on Jan 14, 2020

Aaron1011 commented on Jan 14, 2020

MikailBag commented on Jan 15, 2020

tmiasko commented on Jan 15, 2020

RalfJung commented on Jan 15, 2020

28 remaining items

eddyb commented on Jan 30, 2020

RalfJung commented on Jan 30, 2020

rfcbot commented on Feb 1, 2020

nikomatsakis commented on Feb 3, 2020

pnkfelix commented on Feb 3, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions