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.

3 files changed, 13 insertions, 0 deletions

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