Rollup merge of rust-lang#124281 - RalfJung:win-tls, r=joboet

fix weak memory bug in TLS on Windows

We need to store the `key` *after* we register the dtor.

Now I hope there isn't also some other reason why we have to actually register the dtor last... `@joboet` is there a reason you picked this particular order in rust-lang#102655?

Fixes rust-lang#123583

Author: fmease

library/std/src/sys/pal/windows/thread_local_key.rs

@@ -141,9 +141,15 @@ impl StaticKey {
                     panic!("out of TLS indexes");
                 }
 
-                self.key.store(key + 1, Release);
                 register_dtor(self);
 
+                // Release-storing the key needs to be the last thing we do.
+                // This is because in `fn key()`, other threads will do an acquire load of the key,
+                // and if that sees this write then it will entirely bypass the `InitOnce`. We thus
+                // need to establish synchronization through `key`. In particular that acquire load
+                // must happen-after the register_dtor above, to ensure the dtor actually runs!
+                self.key.store(key + 1, Release);
+
                 let r = c::InitOnceComplete(self.once.get(), 0, ptr::null_mut());
                 debug_assert_eq!(r, c::TRUE);
 
@@ -313,17 +319,29 @@ unsafe fn run_dtors() {
         // Use acquire ordering to observe key initialization.
         let mut cur = DTORS.load(Acquire);
         while !cur.is_null() {
-            let key = (*cur).key.load(Relaxed) - 1;
+            let pre_key = (*cur).key.load(Acquire);
             let dtor = (*cur).dtor.unwrap();
+            cur = (*cur).next.load(Relaxed);
+
+            // In StaticKey::init, we register the dtor before setting `key`.
+            // So if one thread's `run_dtors` races with another thread executing `init` on the same
+            // `StaticKey`, we can encounter a key of 0 here. That means this key was never
+            // initialized in this thread so we can safely skip it.
+            if pre_key == 0 {
+                continue;
+            }
+            // If this is non-zero, then via the `Acquire` load above we synchronized with
+            // everything relevant for this key. (It's not clear that this is needed, since the
+            // release-acquire pair on DTORS also establishes synchronization, but better safe than
+            // sorry.)
+            let key = pre_key - 1;
 
             let ptr = c::TlsGetValue(key);
             if !ptr.is_null() {
                 c::TlsSetValue(key, ptr::null_mut());
                 dtor(ptr as *mut _);
                 any_run = true;
             }
-
-            cur = (*cur).next.load(Relaxed);
         }
 
         if !any_run {