Auto merge of rust-lang#77801 - fusion-engineering-forks:pin-mutex, r=Mark-Simulacrum

Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio

A `sys_common::ReentrantMutex` may not be moved after initializing it with `.init()`. This was not enforced, but only stated as a requirement in the comments on the unsafe functions. This change enforces this no-moving rule using `Pin`, by changing `&self` to a `Pin` in the `init()` and `lock()` functions.

This uncovered a bug I introduced in rust-lang#77154: stdio.rs (the only user of ReentrantMutex) called `init()` on its ReentrantMutexes while constructing them in the intializer of `SyncOnceCell::get_or_init`, which would move them afterwards. Interestingly, the ReentrantMutex unit tests already had the same bug, so this invalid usage has been tested on all (CI-tested) platforms for a long time. Apparently this doesn't break badly on any of the major platforms, but it does break the rules.\*

To be able to keep using SyncOnceCell, this adds a `SyncOnceCell::get_or_init_pin` function, which makes it possible to work with pinned values inside a (pinned) SyncOnceCell. Whether this function should be public or not and what its exact behaviour and interface should be if it would be public is something I'd like to leave for a separate issue or PR. In this PR, this function is internal-only and marked with `pub(crate)`.

\* Note: That bug is now included in 1.48, while this patch can only make it to ~~1.49~~ 1.50. We should consider the implications of 1.48 shipping with a wrong usage of `pthread_mutex_t` / `CRITICAL_SECTION` / .. which technically invokes UB according to their specification. The risk is very low, considering the objects are not 'used' (locked) before the move, and the ReentrantMutex unit tests have verified this works fine in practice.

Edit: This has been backported and included in 1.48. And soon 1.49 too.

---

In future changes, I want to push this usage of Pin further inside `sys` instead of only `sys_common`, and apply it to all 'unmovable' objects there (`Mutex`, `Condvar`, `RwLock`). Also, while `sys_common`'s mutexes and condvars are already taken care of by rust-lang#77147 and rust-lang#77648, its `RwLock` should still be made movable or get pinned.

Author: bors

library/std/src/io/stdio.rs

@@ -9,6 +9,7 @@ use crate::cell::{Cell, RefCell};
 use crate::fmt;
 use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter};
 use crate::lazy::SyncOnceCell;
+use crate::pin::Pin;
 use crate::sync::atomic::{AtomicBool, Ordering};
 use crate::sync::{Arc, Mutex, MutexGuard};
 use crate::sys::stdio;
@@ -490,7 +491,7 @@ pub struct Stdout {
     // FIXME: this should be LineWriter or BufWriter depending on the state of
     //        stdout (tty or not). Note that if this is not line buffered it
     //        should also flush-on-panic or some form of flush-on-abort.
-    inner: &'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>,
+    inner: Pin<&'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>>,
 }
 
 /// A locked reference to the `Stdout` handle.
@@ -550,25 +551,29 @@ pub struct StdoutLock<'a> {
 pub fn stdout() -> Stdout {
     static INSTANCE: SyncOnceCell<ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>> =
         SyncOnceCell::new();
+
+    fn cleanup() {
+        if let Some(instance) = INSTANCE.get() {
+            // Flush the data and disable buffering during shutdown
+            // by replacing the line writer by one with zero
+            // buffering capacity.
+            // We use try_lock() instead of lock(), because someone
+            // might have leaked a StdoutLock, which would
+            // otherwise cause a deadlock here.
+            if let Some(lock) = Pin::static_ref(instance).try_lock() {
+                *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
+            }
+        }
+    }
+
     Stdout {
-        inner: INSTANCE.get_or_init(|| unsafe {
-            let _ = sys_common::at_exit(|| {
-                if let Some(instance) = INSTANCE.get() {
-                    // Flush the data and disable buffering during shutdown
-                    // by replacing the line writer by one with zero
-                    // buffering capacity.
-                    // We use try_lock() instead of lock(), because someone
-                    // might have leaked a StdoutLock, which would
-                    // otherwise cause a deadlock here.
-                    if let Some(lock) = instance.try_lock() {
-                        *lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
-                    }
-                }
-            });
-            let r = ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())));
-            r.init();
-            r
-        }),
+        inner: Pin::static_ref(&INSTANCE).get_or_init_pin(
+            || unsafe {
+                let _ = sys_common::at_exit(cleanup);
+                ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())))
+            },
+            |mutex| unsafe { mutex.init() },
+        ),
     }
 }
 
@@ -700,7 +705,7 @@ impl fmt::Debug for StdoutLock<'_> {
 /// an error.
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct Stderr {
-    inner: &'static ReentrantMutex<RefCell<StderrRaw>>,
+    inner: Pin<&'static ReentrantMutex<RefCell<StderrRaw>>>,
 }
 
 /// A locked reference to the `Stderr` handle.
@@ -756,21 +761,16 @@ pub struct StderrLock<'a> {
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stderr() -> Stderr {
-    // Note that unlike `stdout()` we don't use `Lazy` here which registers a
-    // destructor. Stderr is not buffered nor does the `stderr_raw` type consume
-    // any owned resources, so there's no need to run any destructors at some
-    // point in the future.
-    //
-    // This has the added benefit of allowing `stderr` to be usable during
-    // process shutdown as well!
+    // Note that unlike `stdout()` we don't use `at_exit` here to register a
+    // destructor. Stderr is not buffered , so there's no need to run a
+    // destructor for flushing the buffer
     static INSTANCE: SyncOnceCell<ReentrantMutex<RefCell<StderrRaw>>> = SyncOnceCell::new();
 
     Stderr {
-        inner: INSTANCE.get_or_init(|| unsafe {
-            let r = ReentrantMutex::new(RefCell::new(stderr_raw()));
-            r.init();
-            r
-        }),
+        inner: Pin::static_ref(&INSTANCE).get_or_init_pin(
+            || unsafe { ReentrantMutex::new(RefCell::new(stderr_raw())) },
+            |mutex| unsafe { mutex.init() },
+        ),
     }
 }
 

library/std/src/lazy.rs

@@ -10,6 +10,7 @@ use crate::{
     mem::MaybeUninit,
     ops::{Deref, Drop},
     panic::{RefUnwindSafe, UnwindSafe},
+    pin::Pin,
     sync::Once,
 };
 
@@ -297,6 +298,60 @@ impl<T> SyncOnceCell<T> {
         Ok(unsafe { self.get_unchecked() })
     }
 
+    /// Internal-only API that gets the contents of the cell, initializing it
+    /// in two steps with `f` and `g` if the cell was empty.
+    ///
+    /// `f` is called to construct the value, which is then moved into the cell
+    /// and given as a (pinned) mutable reference to `g` to finish
+    /// initialization.
+    ///
+    /// This allows `g` to inspect an manipulate the value after it has been
+    /// moved into its final place in the cell, but before the cell is
+    /// considered initialized.
+    ///
+    /// # Panics
+    ///
+    /// If `f` or `g` panics, the panic is propagated to the caller, and the
+    /// cell remains uninitialized.
+    ///
+    /// With the current implementation, if `g` panics, the value from `f` will
+    /// not be dropped. This should probably be fixed if this is ever used for
+    /// a type where this matters.
+    ///
+    /// It is an error to reentrantly initialize the cell from `f`. The exact
+    /// outcome is unspecified. Current implementation deadlocks, but this may
+    /// be changed to a panic in the future.
+    pub(crate) fn get_or_init_pin<F, G>(self: Pin<&Self>, f: F, g: G) -> Pin<&T>
+    where
+        F: FnOnce() -> T,
+        G: FnOnce(Pin<&mut T>),
+    {
+        if let Some(value) = self.get_ref().get() {
+            // SAFETY: The inner value was already initialized, and will not be
+            // moved anymore.
+            return unsafe { Pin::new_unchecked(value) };
+        }
+
+        let slot = &self.value;
+
+        // Ignore poisoning from other threads
+        // If another thread panics, then we'll be able to run our closure
+        self.once.call_once_force(|_| {
+            let value = f();
+            // SAFETY: We use the Once (self.once) to guarantee unique access
+            // to the UnsafeCell (slot).
+            let value: &mut T = unsafe { (&mut *slot.get()).write(value) };
+            // SAFETY: The value has been written to its final place in
+            // self.value. We do not to move it anymore, which we promise here
+            // with a Pin<&mut T>.
+            g(unsafe { Pin::new_unchecked(value) });
+        });
+
+        // SAFETY: The inner value has been initialized, and will not be moved
+        // anymore.
+        unsafe { Pin::new_unchecked(self.get_ref().get_unchecked()) }
+    }
+
     /// Consumes the `SyncOnceCell`, returning the wrapped value. Returns
     /// `None` if the cell was empty.
     ///

library/std/src/lib.rs

@@ -266,6 +266,7 @@
 #![feature(format_args_nl)]
 #![feature(gen_future)]
 #![feature(generator_trait)]
+#![feature(get_mut_unchecked)]
 #![feature(global_asm)]
 #![feature(hashmap_internals)]
 #![feature(int_error_internals)]
@@ -293,6 +294,7 @@
 #![feature(panic_info_message)]
 #![feature(panic_internals)]
 #![feature(panic_unwind)]
+#![feature(pin_static_ref)]
 #![feature(prelude_import)]
 #![feature(ptr_internals)]
 #![feature(raw)]

library/std/src/sys_common/remutex.rs

@@ -1,10 +1,10 @@
 #[cfg(all(test, not(target_os = "emscripten")))]
 mod tests;
 
-use crate::fmt;
-use crate::marker;
+use crate::marker::PhantomPinned;
 use crate::ops::Deref;
 use crate::panic::{RefUnwindSafe, UnwindSafe};
+use crate::pin::Pin;
 use crate::sys::mutex as sys;
 
 /// A re-entrant mutual exclusion
@@ -15,6 +15,7 @@ use crate::sys::mutex as sys;
 pub struct ReentrantMutex<T> {
     inner: sys::ReentrantMutex,
     data: T,
+    _pinned: PhantomPinned,
 }
 
 unsafe impl<T: Send> Send for ReentrantMutex<T> {}
@@ -37,10 +38,10 @@ impl<T> RefUnwindSafe for ReentrantMutex<T> {}
 /// guarded data.
 #[must_use = "if unused the ReentrantMutex will immediately unlock"]
 pub struct ReentrantMutexGuard<'a, T: 'a> {
-    lock: &'a ReentrantMutex<T>,
+    lock: Pin<&'a ReentrantMutex<T>>,
 }
 
-impl<T> !marker::Send for ReentrantMutexGuard<'_, T> {}
+impl<T> !Send for ReentrantMutexGuard<'_, T> {}
 
 impl<T> ReentrantMutex<T> {
     /// Creates a new reentrant mutex in an unlocked state.
@@ -51,7 +52,11 @@ impl<T> ReentrantMutex<T> {
     /// once this mutex is in its final resting place, and only then are the
     /// lock/unlock methods safe.
     pub const unsafe fn new(t: T) -> ReentrantMutex<T> {
-        ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t }
+        ReentrantMutex {
+            inner: sys::ReentrantMutex::uninitialized(),
+            data: t,
+            _pinned: PhantomPinned,
+        }
     }
 
     /// Initializes this mutex so it's ready for use.
@@ -60,8 +65,8 @@ impl<T> ReentrantMutex<T> {
     ///
     /// Unsafe to call more than once, and must be called after this will no
     /// longer move in memory.
-    pub unsafe fn init(&self) {
-        self.inner.init();
+    pub unsafe fn init(self: Pin<&mut Self>) {
+        self.get_unchecked_mut().inner.init()
     }
 
     /// Acquires a mutex, blocking the current thread until it is able to do so.
@@ -76,9 +81,9 @@ impl<T> ReentrantMutex<T> {
     /// If another user of this mutex panicked while holding the mutex, then
     /// this call will return failure if the mutex would otherwise be
     /// acquired.
-    pub fn lock(&self) -> ReentrantMutexGuard<'_, T> {
+    pub fn lock(self: Pin<&Self>) -> ReentrantMutexGuard<'_, T> {
         unsafe { self.inner.lock() }
-        ReentrantMutexGuard::new(&self)
+        ReentrantMutexGuard { lock: self }
     }
 
     /// Attempts to acquire this lock.
@@ -93,8 +98,12 @@ impl<T> ReentrantMutex<T> {
     /// If another user of this mutex panicked while holding the mutex, then
     /// this call will return failure if the mutex would otherwise be
     /// acquired.
-    pub fn try_lock(&self) -> Option<ReentrantMutexGuard<'_, T>> {
-        if unsafe { self.inner.try_lock() } { Some(ReentrantMutexGuard::new(&self)) } else { None }
+    pub fn try_lock(self: Pin<&Self>) -> Option<ReentrantMutexGuard<'_, T>> {
+        if unsafe { self.inner.try_lock() } {
+            Some(ReentrantMutexGuard { lock: self })
+        } else {
+            None
+        }
     }
 }
 
@@ -107,30 +116,6 @@ impl<T> Drop for ReentrantMutex<T> {
     }
 }
 
-impl<T: fmt::Debug + 'static> fmt::Debug for ReentrantMutex<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self.try_lock() {
-            Some(guard) => f.debug_struct("ReentrantMutex").field("data", &*guard).finish(),
-            None => {
-                struct LockedPlaceholder;
-                impl fmt::Debug for LockedPlaceholder {
-                    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-                        f.write_str("<locked>")
-                    }
-                }
-
-                f.debug_struct("ReentrantMutex").field("data", &LockedPlaceholder).finish()
-            }
-        }
-    }
-}
-
-impl<'mutex, T> ReentrantMutexGuard<'mutex, T> {
-    fn new(lock: &'mutex ReentrantMutex<T>) -> ReentrantMutexGuard<'mutex, T> {
-        ReentrantMutexGuard { lock }
-    }
-}
-
 impl<T> Deref for ReentrantMutexGuard<'_, T> {
     type Target = T;
 

library/std/src/sys_common/remutex/tests.rs

@@ -1,15 +1,18 @@
+use crate::boxed::Box;
 use crate::cell::RefCell;
+use crate::pin::Pin;
 use crate::sync::Arc;
 use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
 use crate::thread;
 
 #[test]
 fn smoke() {
     let m = unsafe {
-        let m = ReentrantMutex::new(());
-        m.init();
+        let mut m = Box::pin(ReentrantMutex::new(()));
+        m.as_mut().init();
         m
     };
+    let m = m.as_ref();
     {
         let a = m.lock();
         {
@@ -27,18 +30,19 @@ fn smoke() {
 #[test]
 fn is_mutex() {
     let m = unsafe {
-        let m = Arc::new(ReentrantMutex::new(RefCell::new(0)));
-        m.init();
-        m
+        // FIXME: Simplify this if Arc gets a Arc::get_pin_mut.
+        let mut m = Arc::new(ReentrantMutex::new(RefCell::new(0)));
+        Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init();
+        Pin::new_unchecked(m)
     };
     let m2 = m.clone();
-    let lock = m.lock();
+    let lock = m.as_ref().lock();
     let child = thread::spawn(move || {
-        let lock = m2.lock();
+        let lock = m2.as_ref().lock();
         assert_eq!(*lock.borrow(), 4950);
     });
     for i in 0..100 {
-        let lock = m.lock();
+        let lock = m.as_ref().lock();
         *lock.borrow_mut() += i;
     }
     drop(lock);
@@ -48,20 +52,21 @@ fn is_mutex() {
 #[test]
 fn trylock_works() {
     let m = unsafe {
-        let m = Arc::new(ReentrantMutex::new(()));
-        m.init();
-        m
+        // FIXME: Simplify this if Arc gets a Arc::get_pin_mut.
+        let mut m = Arc::new(ReentrantMutex::new(()));
+        Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init();
+        Pin::new_unchecked(m)
     };
     let m2 = m.clone();
-    let _lock = m.try_lock();
-    let _lock2 = m.try_lock();
+    let _lock = m.as_ref().try_lock();
+    let _lock2 = m.as_ref().try_lock();
     thread::spawn(move || {
-        let lock = m2.try_lock();
+        let lock = m2.as_ref().try_lock();
         assert!(lock.is_none());
     })
     .join()
     .unwrap();
-    let _lock3 = m.try_lock();
+    let _lock3 = m.as_ref().try_lock();
 }
 
 pub struct Answer<'a>(pub ReentrantMutexGuard<'a, RefCell<u32>>);