Fix race condition in feature cache on 32 platforms

If we observe that the second word is initialized, we can't really
assume that the first is initialized as well. So check each word
separately.

Author: matklad

crates/std_detect/src/detect/cache.rs

@@ -5,11 +5,7 @@
 
 use crate::sync::atomic::Ordering;
 
-#[cfg(target_pointer_width = "64")]
-use crate::sync::atomic::AtomicU64;
-
-#[cfg(target_pointer_width = "32")]
-use crate::sync::atomic::AtomicU32;
+use crate::sync::atomic::AtomicUsize;
 
 /// Sets the `bit` of `x`.
 #[inline]
@@ -30,7 +26,7 @@ const fn unset_bit(x: u64, bit: u32) -> u64 {
 }
 
 /// Maximum number of features that can be cached.
-const CACHE_CAPACITY: u32 = 63;
+const CACHE_CAPACITY: u32 = 62;
 
 /// This type is used to initialize the cache
 #[derive(Copy, Clone)]
@@ -81,83 +77,48 @@ impl Initializer {
 }
 
 /// This global variable is a cache of the features supported by the CPU.
-static CACHE: Cache = Cache::uninitialized();
-
-/// Feature cache with capacity for `CACHE_CAPACITY` features.
-///
-/// Note: the last feature bit is used to represent an
-/// uninitialized cache.
-#[cfg(target_pointer_width = "64")]
-struct Cache(AtomicU64);
-
 #[cfg(target_pointer_width = "64")]
-#[allow(clippy::use_self)]
-impl Cache {
-    /// Creates an uninitialized cache.
-    #[allow(clippy::declare_interior_mutable_const)]
-    const fn uninitialized() -> Self {
-        Cache(AtomicU64::new(u64::max_value()))
-    }
-    /// Is the cache uninitialized?
-    #[inline]
-    pub(crate) fn is_uninitialized(&self) -> bool {
-        self.0.load(Ordering::Relaxed) == u64::max_value()
-    }
-
-    /// Is the `bit` in the cache set?
-    #[inline]
-    pub(crate) fn test(&self, bit: u32) -> bool {
-        test_bit(CACHE.0.load(Ordering::Relaxed), bit)
-    }
+static CACHE: [Cache; 1] = [Cache::uninitialized()];
 
-    /// Initializes the cache.
-    #[inline]
-    pub(crate) fn initialize(&self, value: Initializer) {
-        self.0.store(value.0, Ordering::Relaxed);
-    }
-}
+/// This global variable is a cache of the features supported by the CPU.
+#[cfg(target_pointer_width = "32")]
+static CACHE: [Cache; 2] = [Cache::uninitialized(), Cache::uninitialized()];
 
-/// Feature cache with capacity for `CACHE_CAPACITY` features.
+/// Feature cache with capacity for `usize::max_value() - 1` features.
 ///
 /// Note: the last feature bit is used to represent an
 /// uninitialized cache.
-#[cfg(target_pointer_width = "32")]
-struct Cache(AtomicU32, AtomicU32);
+///
+/// Note: we use `Relaxed` atomic operations, because we are only interested
+/// in the effects of operations on a single memory location. That is, we only
+/// need "modification order", and not the full-blown "happens before".
+struct Cache(AtomicUsize);
 
-#[cfg(target_pointer_width = "32")]
 impl Cache {
     /// Creates an uninitialized cache.
+    #[allow(clippy::declare_interior_mutable_const)]
     const fn uninitialized() -> Self {
-        Cache(
-            AtomicU32::new(u32::max_value()),
-            AtomicU32::new(u32::max_value()),
-        )
+        Cache(AtomicUsize::new(usize::max_value()))
     }
     /// Is the cache uninitialized?
     #[inline]
     pub(crate) fn is_uninitialized(&self) -> bool {
-        self.1.load(Ordering::Relaxed) == u32::max_value()
+        self.0.load(Ordering::Relaxed) == usize::max_value()
     }
 
     /// Is the `bit` in the cache set?
     #[inline]
     pub(crate) fn test(&self, bit: u32) -> bool {
-        if bit < 32 {
-            test_bit(CACHE.0.load(Ordering::Relaxed) as u64, bit)
-        } else {
-            test_bit(CACHE.1.load(Ordering::Relaxed) as u64, bit - 32)
-        }
+        test_bit(self.0.load(Ordering::Relaxed) as u64, bit)
     }
 
     /// Initializes the cache.
     #[inline]
-    pub(crate) fn initialize(&self, value: Initializer) {
-        let lo: u32 = value.0 as u32;
-        let hi: u32 = (value.0 >> 32) as u32;
-        self.0.store(lo, Ordering::Relaxed);
-        self.1.store(hi, Ordering::Relaxed);
+    fn initialize(&self, value: usize) {
+        self.0.store(value, Ordering::Relaxed);
     }
 }
+
 cfg_if::cfg_if! {
     if #[cfg(feature = "std_detect_env_override")] {
         #[inline(never)]
@@ -167,16 +128,32 @@ cfg_if::cfg_if! {
                     let _ = super::Feature::from_str(v).map(|v| value.unset(v as u32));
                 }
             }
-            CACHE.initialize(value);
+            do_initialize(value);
         }
     } else {
         #[inline]
         fn initialize(value: Initializer) {
-            CACHE.initialize(value);
+            do_initialize(value);
         }
     }
 }
 
+#[cfg(target_pointer_width = "32")]
+const CACHE_BITS: u32 = 31;
+#[cfg(target_pointer_width = "64")]
+const CACHE_BITS: u32 = 63;
+
+const CACHE_MASK: usize = 1 << CACHE_BITS - 1;
+
+#[inline]
+fn do_initialize(value: Initializer) {
+    CACHE[0].initialize((value.0) as usize & CACHE_MASK);
+    #[cfg(target_pointer_width = "32")]
+    {
+        CACHE[1].initialize((value.0 >> CACHE_BITS) as usize & CACHE_MASK);
+    }
+}
+
 /// Tests the `bit` of the storage. If the storage has not been initialized,
 /// initializes it with the result of `f()`.
 ///
@@ -194,8 +171,18 @@ pub(crate) fn test<F>(bit: u32, f: F) -> bool
 where
     F: FnOnce() -> Initializer,
 {
-    if CACHE.is_uninitialized() {
-        initialize(f());
+    #[cfg(target_pointer_width = "32")]
+    {
+        if bit >= CACHE_BITS {
+            if CACHE[1].is_uninitialized() {
+                initialize(f())
+            }
+            return CACHE[1].test(bit - CACHE_BITS);
+        }
+    }
+
+    if CACHE[0].is_uninitialized() {
+        initialize(f())
     }
-    CACHE.test(bit)
+    CACHE[0].test(bit)
 }