Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gksato wants to merge 2 commits into
base: main
Choose a base branch
from
+42 −0
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1033c71
Doc: `MaybeUninit::assume_init_read` Safety: warn on thread safety
gksato Nov 2, 2025
59042f3
Reword `MaybeUninit::assume_init_read` doc
gksato Nov 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions library/core/src/mem/maybe_uninit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,15 +726,31 @@ impl<T> MaybeUninit<T> {
/// `assume_init_read` and then [`assume_init`]), it is your responsibility
/// to ensure that data may indeed be duplicated.
///
/// 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`. You are responsible for guaranteeing
/// the thread safety of the transfer. Note that `MaybeUninit<T>` is [`Sync`] if `T`
/// is merely [`Sync`]; therefore, safely obtaining `&MaybeUninit<T>` does *not* ensure
/// the said thread safety. If `T` is [`Send`], the thread safety is
/// guaranteed; otherwise, you will need to provide type-specific reasoning to
/// prove that the transfer and your usage of the value `T` are actually thread-safe.
/// If your type's safe API relies on this function, you may need to manually
/// implement `!Send`, `!Sync`, [`Send`], and/or [`Sync`] to *tighten* the conditions
/// for the type to be [`Send`] and/or [`Sync`].
Comment on lines +733 to +740
Copy link
Member

@Amanieu Amanieu Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@gksato gksato Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Amanieu Amanieu Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@gksato gksato Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Amanieu Amanieu Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@gksato gksato Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Amanieu Amanieu Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@gksato gksato Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; });
    })
}

Copy link
Author

@gksato gksato Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
        })
    });
}

Copy link
Member

@Amanieu Amanieu Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// [inv]: #initialization-invariant
/// [`assume_init`]: MaybeUninit::assume_init
/// [`Send`]: crate::marker::Send
/// [`Sync`]: crate::marker::Sync
///
/// # Examples
///
/// Correct usage of this method:
///
/// ```rust
/// use std::mem::MaybeUninit;
/// use std::thread;
///
/// let mut x = MaybeUninit::<u32>::uninit();
/// x.write(13);
Expand All @@ -749,19 +765,45 @@ impl<T> MaybeUninit<T> {
/// // Duplicating a `None` value is okay, so we may read multiple times.
/// let x2 = unsafe { x.assume_init_read() };
/// assert_eq!(x1, x2);
///
/// let mut x = MaybeUninit::<Vec<u32>>::uninit();
/// x.write(vec![0, 1, 2]);
/// // Moving the vector to another thread. We may read only once.
/// // `Vec<u32>` is `Send`, so this is thread-safe.
/// thread::scope(|s| {
/// s.spawn(|| {
/// let v = unsafe { x.assume_init_read() };
/// assert_eq!(v, vec![0, 1, 2]);
/// });
/// });
/// ```
///
/// *Incorrect* usage of this method:
///
/// ```rust,no_run
/// use std::mem::MaybeUninit;
/// use std::thread;
/// use std::sync::Mutex;
///
/// let mut x = MaybeUninit::<Option<Vec<u32>>>::uninit();
/// x.write(Some(vec![0, 1, 2]));
/// let x1 = unsafe { x.assume_init_read() };
/// let x2 = unsafe { x.assume_init_read() };
/// // We now created two copies of the same vector, leading to a double-free ⚠️ when
/// // they both get dropped!
///
/// let mtx = Mutex::new(0u32);
/// let x = MaybeUninit::new(mtx.lock().unwrap());
/// // Moving the `MutexGuard<'_, u32>: !Send + Sync` to another thread,
/// // thread safety not guaranteed here.
/// thread::scope(|s| {
/// // This compiles because `MaybeUninit<MutexGuard<'_, u32>>` is `Sync`.
/// s.spawn(|| {
/// let _unused = unsafe { x.assume_init_read() };
/// // `_unused: MutexGuard<'_, u32>` is dropped here, on a different thread
/// // than the one that locked the mutex; this is library-level UB ⚠️!
/// });
/// });
/// ```
#[stable(feature = "maybe_uninit_extra", since = "1.60.0")]
#[rustc_const_stable(feature = "const_maybe_uninit_assume_init_read", since = "1.75.0")]
Expand Down
Loading