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]

r? Amanieu