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

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