Doc: MaybeUninit::assume_init_read Safety: warn on thread safety

[gksato]

Fixes #148083.

Due to impl<T: Sync> Sync for MaybeUninit<T>,
MaybeUninit::assume_init_read is prone to the illegal use transferring ownership of !Send + Sync value across threads. This PR adds a warning on this thread safety problem to the method's Safety section. This PR also adds two code examples related to this problem.

On the bad case code example: I wish I could use something better than MutexGuard<'_, u32>, because I wasn't able to produce Miri-detectable UB. core::sync::Exclusive<&Cell<u32>> would be nice, but it's unstable.

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[gksato]

@rustbot label A-docs I-unsound

[gksato]

@rustbot label -I-unsound -I-prioritize

[gksato]

Is it worth adding the following, or something similar?

Note that, if you duplicate the ownership of a non-Copy value with this function (most typically, by calling assume_init_* after this without calling write), it constitutes a fundamental breakage of Rust's safety constraint. In that case, neither Send or Sync can guarantee thread safety.

[gksato]

@Amanieu Can I re-set you as the reviewer? I know you all have loads of work to do, but it's been three weeks and I'd like to know if re-assigning helps to make review process easier.

[Amanieu]

The text after "Note that MaybeUninit<T> is Sync" gets a bit confusing because it tries to explore all the combinations of [!]Send and [!]Sync. Instead this should only focus on the problematic combination which is T: !Send + Sync, where you need to manually ensure that you are maintaining the Send invariants.

[gksato]

It took time, and I finally came up with something worth consideration:

the thread safety of the transfer. The main problematic case is T: Sync + !Send. In this situation, T: Sync gives MaybeUninit<T>: Sync, so you can freely share &MaybeUninit<T> across threads through safe operations. However, actually reading out T with assume_init_read is not guaranteed to be thread-safe because T is not Send. In this case, the type system no longer enforces the usual thread-safety guarantees, so you must rely on your own reasoning that this transfer and the subsequent uses of T are thread-safe.

If your type's safe API can call this method on Sync + !Send types, note that keeping its auto-implementations of Send/Sync may be unsound. You might need to manually implement Sync and/or Send to ensure that any cross-thread ownership transfer of T through your type requires T: Send. Generally, safe cross-thread ownership transfer of T requires T: Send, regardless of whether T: Sync or whether MaybeUninit<T>: Sync.

[Amanieu]

Here's the wording I came up with. Let me know your thoughts.

Since this function does not require T: Send, you also need to carefully consider thread safety. If you call this function from a different thread than the one that holds the MaybeUninit<T>, it logically constitutes a cross-thread ownership transfer of the contained value T. This can be problematic when T: Sync + !Send because Sync allows &MaybeUninit<T> to be safely shared across threads. In this case, the type system no longer enforces the usual thread-safety guarantees, so you must rely on your own reasoning that this transfer and the subsequent uses of T are thread-safe.

If a safe API can result in this function being called on Sync + !Send types, then keeping its auto-implementations of Send/Sync may be unsound. To avoid this, you can manually add T: Send bounds to the callers of this function. Generally, safe cross-thread ownership transfer of T requires T: Send, regardless of whether T: Sync or whether MaybeUninit<T>: Sync.

[gksato]

In this case, the type system no...

The first paragraph, as a whole, seems like a wonderful simplification to me. Since I love verboseness, I might add:

In this case, for (as/since) T: !Send, the type system no longer enforces...

However I guess it depends on preference. Brevity might win. What do you think?

On the second paragraph... hmm... to me it is puzzling. It required back-tracking when I read it. I was thinking of, like,

pub mod take_once {
    use core::mem::{self, MaybeUninit};
    use core::sync::atomic::{self, AtomicBool};

    /// lock-free Mutex<Option<T>>, but you can only take ownership of the inner value;
    /// you can't write, insert or borrow anything that's inside.
    pub struct TakeOnce<T> {
        alive: AtomicBool,
        inner: MaybeUninit<T>,
    }

    impl<T> TakeOnce<T> {
        pub fn new(t: T) -> Self {
            Self {
                alive: AtomicBool::new(true),
                inner: MaybeUninit::new(t),
            }
        }
        pub fn take(&self) -> Option<T> {
            if self.alive.swap(false, atomic::Ordering::Relaxed) {
                unsafe { Some(self.inner.assume_init_read()) }
            } else {
                None
            }
        }
    }

    impl<T> Drop for TakeOnce<T> {
        fn drop(&mut self) {
            if mem::replace(self.alive.get_mut(), false) {
                unsafe {
                    self.inner.assume_init_drop();
                }
            }
        }
    }

    // The following impl is MANDATORY: without this, `self.take()` is unsound
    unsafe impl<T: Send> Sync for TakeOnce<T> {}
}

I meant:

If your type's (TakeOnce's) safe API calls this method on Sync+!Send types (T), then keeping its (TakeOnce's) auto-implementations of Send/Sync may be unsound.

On the other hand, I failed to see what you meant at glance.

If a safe API can result in this function being called on Sync + !Send types, then keeping its auto-implementations of Send/Sync may be unsound.

What do you mean by "its auto implementations"? I assume "its" points to the singular "a safe API", but "a safe API" can be a method of a type or a function in the wild... what did you intend that would have some implementations of Send/Sync?

[Amanieu]

Sorry for losing track of this PR in the last few weeks.

What I'm trying to say in that paragraph is that if you are using a MaybeUninit<T> internally in your code, then in the safe wrapper around the MaybeUninit<T> (i.e. your code) you need to add T: Send bound on the methods that use assume_init_read to move a value. This essentially boils down to making sure you add T: Send as an extra bound on any function that calls assume_init_read.

[gksato]

Sorry for losing track of this PR in the last few weeks.

No worries. You all are going through a lot, I know.

You need additional bounds, not impls.

I assume you mean that we must stress bounds on functions (not that impls on wrappers are never useful). It previously didn't occurred to me, but when you mention it, it is completely obvious that it is the function bounds that should come as the primary choice. I agree with that.

Whether we should remove references to impls... well, I am biased, for I love impls, because I found this issue when I was writing TakeOnce. It deserves impl, not bounds on take(&self), because TakeOnce<T> is a lock-free Mutex<Option<T>> with limited functionality.

What about:

If a safe function ends up calling this method for generic T, the easiest and safest way to ensure thread safety is adding a T: Send bound on the function. We strongly recommend this solution; other solutions, such as wrapping MaybeUninit<T> in a type and manually implementing Send/Sync on the wrapper to tighten the auto-implementation's bounds, require subtle reasoning to do soundly and should be considered niche. Generally, safe cross-thread ownership transfer of T requires T: Send, regardless of whether T: Sync or whether MaybeUninit<T>: Sync.

[Amanieu]

I think that I understand where the confusion is coming from. In your TakeOnce example you are adding a Sync bound, but remember that this is in addition to the implicit Sync bound on TakeOnce that comes from its field being Sync. This results in TakeOnce being Sync when T is either Send or Sync, which is not what you want.

The correct approach for your example is to suppress the implicit Sync bound by placing inner in an UnsafeCell. Then your explicit bound will only give Sync when T: Send. In a way, you can think of the UnsafeCell as allowing you to "mutate" the data by moving it, which normally requires inner mutability.

[gksato]

This results in TakeOnce being Sync when T is either Send or Sync, which is not what you want.

Well... I'm afraid I don't think so. Manually implementing Sync/Send for a type completely nullifies their default auto-implementation for it. Even if a type only wraps Sync types, it doesn't imply that the outer type is Sync if it has a manual implementation of Sync. Here is an illustrating example, this doesn't compile (See Playground):

use core::marker::PhantomData;
#[derive(Copy, Clone)]
// !Send + Sync type
struct Typ(PhantomData<*mut ()>);

impl Typ {
    fn new() -> Self {
        Typ(PhantomData)
    }
}

unsafe impl Sync for Typ {}

struct F<T>(T);
// F<T> is Sync if T:Send. Let's see what happens if T: Sync...
unsafe impl<T: Send> Sync for F<T> {}

fn main() {
    use std::thread;
    let t = F(Typ::new());
    let r = &t;
    // doesn't compile because F<T>: !Sync
    thread::scope(|scp| {
        scp.spawn(|| { let u = r; });
    })
}

[gksato]

Also, see that the following doesn't compile, either (Playground):

pub mod take_once {
    use core::mem::{self, MaybeUninit};
    use core::sync::atomic::{self, AtomicBool};

    /// lock-free Mutex<Option<T>>, but you can only take ownership of the inner value;
    /// you can't write, insert or borrow anything that's inside.
    pub struct TakeOnce<T> {
        alive: AtomicBool,
        inner: MaybeUninit<T>,
    }

    impl<T> TakeOnce<T> {
        pub fn new(t: T) -> Self {
            Self {
                alive: AtomicBool::new(true),
                inner: MaybeUninit::new(t),
            }
        }
        pub fn take(&self) -> Option<T> {
            if self.alive.swap(false, atomic::Ordering::Relaxed) {
                unsafe { Some(self.inner.assume_init_read()) }
            } else {
                None
            }
        }
    }

    impl<T> Drop for TakeOnce<T> {
        fn drop(&mut self) {
            if mem::replace(self.alive.get_mut(), false) {
                unsafe {
                    self.inner.assume_init_drop();
                }
            }
        }
    }

    // The following impl is MANDATORY: without this, `self.take()` is unsound
    unsafe impl<T: Send> Sync for TakeOnce<T> {}
}

use core::marker::PhantomData;
#[derive(Copy, Clone)]
// Sync + !Send type
struct Typ(PhantomData<*mut ()>);

unsafe impl Sync for Typ {}

fn main() {
    use take_once::TakeOnce;
    use std::thread;
    let typ = TakeOnce::new(Typ(PhantomData));
    
    // `typ` isn't `Sync`; so fails to compile
    thread::scope(|scope| {
        scope.spawn(||{
            typ.take();
        })
    });
}

[Amanieu]

You're right, it seems I was wrong about how auto-traits work.

So back to the main issue, it seems that there are several ways of handling T: !Send + Sync types:

• Ensure you only call assume_init_read from function that have a mutable reference to the MaybeUninit<T>, which only requires Send, not Sync.
• Ensure your wrapper only implements Sync when T: Send by explicitly overriding the default Sync impl.
• Have the MaybeUninit<T> in some form of cell, which suppresses the default Sync impl. It is then up to you to decide when you want your type to be Sync.
• Require T: Copy, which allows moving a value with just &T and therefore makes the Send/Sync distinction irrelevant.

[Amanieu]

r? Amanieu