Auto merge of #53108 - RalfJung:mutex, r=alexcrichton

clarify partially initialized Mutex issues

Using a `sys_common::mutex::Mutex` without calling `init` is dangerous, and yet there are some places that do this. I tried to find all of them and add an appropriate comment about reentrancy.

I found two places where (I think) reentrancy can actually occur, and was not able to come up with an argument for why this is okay. Someone who knows `io::lazy` and/or `sys_common::at_exit_imp` should have a careful look at this.

9 files changed, 38 insertions, 6 deletions

diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs
index d357966be92..4fb367fb6ba 100644
--- a/src/libstd/io/lazy.rs
+++ b/src/libstd/io/lazy.rs
@@ -15,6 +15,7 @@ use sys_common;
 use sys_common::mutex::Mutex;
 
 pub struct Lazy<T> {
+    // We never call `lock.init()`, so it is UB to attempt to acquire this mutex reentrantly!
     lock: Mutex,
     ptr: Cell<*mut Arc<T>>,
     init: fn() -> Arc<T>,
@@ -26,7 +27,9 @@ const fn done<T>() -> *mut Arc<T> { 1_usize as *mut _ }
 unsafe impl<T> Sync for Lazy<T> {}
 
 impl<T: Send + Sync + 'static> Lazy<T> {
-    pub const fn new(init: fn() -> Arc<T>) -> Lazy<T> {
+    /// Safety: `init` must not call `get` on the variable that is being
+    /// initialized.
+    pub const unsafe fn new(init: fn() -> Arc<T>) -> Lazy<T> {
         Lazy {
             lock: Mutex::new(),
             ptr: Cell::new(ptr::null_mut()),
@@ -48,6 +51,7 @@ impl<T: Send + Sync + 'static> Lazy<T> {
         }
     }
 
+    // Must only be called with `lock` held
     unsafe fn init(&'static self) -> Arc<T> {
         // If we successfully register an at exit handler, then we cache the
         // `Arc` allocation in our own internal box (it will get deallocated by
@@ -60,6 +64,9 @@ impl<T: Send + Sync + 'static> Lazy<T> {
             };
             drop(Box::from_raw(ptr))
         });
+        // This could reentrantly call `init` again, which is a problem
+        // because our `lock` allows reentrancy!
+        // That's why `new` is unsafe and requires the caller to ensure no reentrancy happens.
         let ret = (self.init)();
         if registered.is_ok() {
             self.ptr.set(Box::into_raw(Box::new(ret.clone())));
diff --git a/src/libstd/io/stdio.rs b/src/libstd/io/stdio.rs
index fffe8fc559b..1f256f518c7 100644
--- a/src/libstd/io/stdio.rs
+++ b/src/libstd/io/stdio.rs
@@ -197,12 +197,13 @@ pub struct StdinLock<'a> {
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stdin() -> Stdin {
-    static INSTANCE: Lazy<Mutex<BufReader<Maybe<StdinRaw>>>> = Lazy::new(stdin_init);
+    static INSTANCE: Lazy<Mutex<BufReader<Maybe<StdinRaw>>>> = unsafe { Lazy::new(stdin_init) };
     return Stdin {
         inner: INSTANCE.get().expect("cannot access stdin during shutdown"),
     };
 
     fn stdin_init() -> Arc<Mutex<BufReader<Maybe<StdinRaw>>>> {
+        // This must not reentrantly access `INSTANCE`
         let stdin = match stdin_raw() {
             Ok(stdin) => Maybe::Real(stdin),
             _ => Maybe::Fake
@@ -396,12 +397,13 @@ pub struct StdoutLock<'a> {
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stdout() -> Stdout {
     static INSTANCE: Lazy<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>>
-        = Lazy::new(stdout_init);
+        = unsafe { Lazy::new(stdout_init) };
     return Stdout {
         inner: INSTANCE.get().expect("cannot access stdout during shutdown"),
     };
 
     fn stdout_init() -> Arc<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>> {
+        // This must not reentrantly access `INSTANCE`
         let stdout = match stdout_raw() {
             Ok(stdout) => Maybe::Real(stdout),
             _ => Maybe::Fake,
@@ -531,12 +533,14 @@ pub struct StderrLock<'a> {
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn stderr() -> Stderr {
-    static INSTANCE: Lazy<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> = Lazy::new(stderr_init);
+    static INSTANCE: Lazy<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> =
+        unsafe { Lazy::new(stderr_init) };
     return Stderr {
         inner: INSTANCE.get().expect("cannot access stderr during shutdown"),
     };
 
     fn stderr_init() -> Arc<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> {
+        // This must not reentrantly access `INSTANCE`
         let stderr = match stderr_raw() {
             Ok(stderr) => Maybe::Real(stderr),
             _ => Maybe::Fake,
diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs
index 7e32ec1347e..c3c033dfbc7 100644
--- a/src/libstd/sys/unix/args.rs
+++ b/src/libstd/sys/unix/args.rs
@@ -80,6 +80,8 @@ mod imp {
 
     static mut ARGC: isize = 0;
     static mut ARGV: *const *const u8 = ptr::null();
+    // We never call `ENV_LOCK.init()`, so it is UB to attempt to
+    // acquire this mutex reentrantly!
     static LOCK: Mutex = Mutex::new();
 
     pub unsafe fn init(argc: isize, argv: *const *const u8) {
diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs
index 60b03cdbeb0..1d447de1134 100644
--- a/src/libstd/sys/unix/mutex.rs
+++ b/src/libstd/sys/unix/mutex.rs
@@ -25,8 +25,10 @@ unsafe impl Sync for Mutex {}
 #[allow(dead_code)] // sys isn't exported yet
 impl Mutex {
     pub const fn new() -> Mutex {
-        // Might be moved and address is changing it is better to avoid
-        // initialization of potentially opaque OS data before it landed
+        // Might be moved to a different address, so it is better to avoid
+        // initialization of potentially opaque OS data before it landed.
+        // Be very careful using this newly constructed `Mutex`, reentrant
+        // locking is undefined behavior until `init` is called!
         Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
     }
     #[inline]
diff --git a/src/libstd/sys/unix/os.rs b/src/libstd/sys/unix/os.rs
index 1d92e8fc97c..08c3e154978 100644
--- a/src/libstd/sys/unix/os.rs
+++ b/src/libstd/sys/unix/os.rs
@@ -33,6 +33,8 @@ use sys::fd;
 use vec;
 
 const TMPBUF_SZ: usize = 128;
+// We never call `ENV_LOCK.init()`, so it is UB to attempt to
+// acquire this mutex reentrantly!
 static ENV_LOCK: Mutex = Mutex::new();
 
 
diff --git a/src/libstd/sys_common/at_exit_imp.rs b/src/libstd/sys_common/at_exit_imp.rs
index b28a4d2f8be..76e5df2c865 100644
--- a/src/libstd/sys_common/at_exit_imp.rs
+++ b/src/libstd/sys_common/at_exit_imp.rs
@@ -23,6 +23,8 @@ type Queue = Vec<Box<dyn FnBox()>>;
 // on poisoning and this module needs to operate at a lower level than requiring
 // the thread infrastructure to be in place (useful on the borders of
 // initialization/destruction).
+// We never call `LOCK.init()`, so it is UB to attempt to
+// acquire this mutex reentrantly!
 static LOCK: Mutex = Mutex::new();
 static mut QUEUE: *mut Queue = ptr::null_mut();
 
@@ -61,6 +63,7 @@ pub fn cleanup() {
             if !queue.is_null() {
                 let queue: Box<Queue> = Box::from_raw(queue);
                 for to_run in *queue {
+                    // We are not holding any lock, so reentrancy is fine.
                     to_run();
                 }
             }
@@ -72,6 +75,8 @@ pub fn push(f: Box<dyn FnBox()>) -> bool {
     unsafe {
         let _guard = LOCK.lock();
         if init() {
+            // We are just moving `f` around, not calling it.
+            // There is no possibility of reentrancy here.
             (*QUEUE).push(f);
             true
         } else {
diff --git a/src/libstd/sys_common/mutex.rs b/src/libstd/sys_common/mutex.rs
index 608355b7d70..c6d531c7a1a 100644
--- a/src/libstd/sys_common/mutex.rs
+++ b/src/libstd/sys_common/mutex.rs
@@ -24,11 +24,17 @@ impl Mutex {
     ///
     /// Behavior is undefined if the mutex is moved after it is
     /// first used with any of the functions below.
+    /// Also, until `init` is called, behavior is undefined if this
+    /// mutex is ever used reentrantly, i.e., `raw_lock` or `try_lock`
+    /// are called by the thread currently holding the lock.
     pub const fn new() -> Mutex { Mutex(imp::Mutex::new()) }
 
     /// Prepare the mutex for use.
     ///
     /// This should be called once the mutex is at a stable memory address.
+    /// If called, this must be the very first thing that happens to the mutex.
+    /// Calling it in parallel with or after any operation (including another
+    /// `init()`) is undefined behavior.
     #[inline]
     pub unsafe fn init(&mut self) { self.0.init() }
 
diff --git a/src/libstd/sys_common/thread_local.rs b/src/libstd/sys_common/thread_local.rs
index 75f6b9ac7fd..bb72cb0930a 100644
--- a/src/libstd/sys_common/thread_local.rs
+++ b/src/libstd/sys_common/thread_local.rs
@@ -161,6 +161,8 @@ impl StaticKey {
         // Additionally a 0-index of a tls key hasn't been seen on windows, so
         // we just simplify the whole branch.
         if imp::requires_synchronized_create() {
+            // We never call `INIT_LOCK.init()`, so it is UB to attempt to
+            // acquire this mutex reentrantly!
             static INIT_LOCK: Mutex = Mutex::new();
             let _guard = INIT_LOCK.lock();
             let mut key = self.key.load(Ordering::SeqCst);
diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs
index bbe80df7e8b..61c6084a250 100644
--- a/src/libstd/thread/mod.rs
+++ b/src/libstd/thread/mod.rs
@@ -940,6 +940,8 @@ pub struct ThreadId(u64);
 impl ThreadId {
     // Generate a new unique thread ID.
     fn new() -> ThreadId {
+        // We never call `GUARD.init()`, so it is UB to attempt to
+        // acquire this mutex reentrantly!
         static GUARD: mutex::Mutex = mutex::Mutex::new();
         static mut COUNTER: u64 = 0;