Fix thread-safety in GC, type cache, and instruction cache

GC / refcount: - Add safe_inc() check for strong()==0 in RefCount - Add try_to_owned() to PyObject for atomic refcount acquire - Replace strong_count()+to_owned() with try_to_owned() in GC collection and weakref callback paths to prevent TOCTOU races Type cache: - Add proper SeqLock (sequence counter) to TypeCacheEntry - Readers spin-wait on odd sequence, validate after read - Writers bracket updates with begin_write/end_write - Use try_to_owned + pointer revalidation on read path - Call modified() BEFORE attribute modification in set_attr Instruction cache: - Add pointer_cache (AtomicUsize array) to CodeUnits for single atomic pointer load/store (prevents torn reads) - Add try_read_cached_descriptor with try_to_owned + pointer and version revalidation after increment - Add write_cached_descriptor with version-bracketed writes RLock: - Fix release() to check is_owned_by_current_thread - Add _release_save/_acquire_restore methods
youknowone · Mar 5, 2026 · Mar 5, 2026 · Mar 5, 2026 · Mar 5, 2026 · Mar 5, 2026
commit 70169e8858fa7062a1b0dcc244688ef2fc39b377
diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py
@@ -4815,8 +4815,6 @@ def test_finalize(self):
         result = [obj for obj in iter(conn.recv, 'STOP')]
         self.assertEqual(result, ['a', 'b', 'd10', 'd03', 'd02', 'd01', 'e'])
 
-    # TODO: RUSTPYTHON - gc.get_threshold() and gc.set_threshold() not implemented
-    @unittest.expectedFailure
     @support.requires_resource('cpu')
     def test_thread_safety(self):
         # bpo-24484: _run_finalizers() should be thread-safe

diff --git a/crates/common/src/refcount.rs b/crates/common/src/refcount.rs
@@ -123,7 +123,7 @@ impl RefCount {
     pub fn safe_inc(&self) -> bool {
         let mut old = State::from_raw(self.state.load(Ordering::Relaxed));
         loop {
-            if old.destructed() {
+            if old.destructed() || old.strong() == 0 {
                 return false;
             }
             if (old.strong() as usize) >= STRONG {

diff --git a/crates/compiler-core/src/bytecode.rs b/crates/compiler-core/src/bytecode.rs
@@ -12,7 +12,7 @@ use core::{
     cell::UnsafeCell,
     hash, mem,
     ops::Deref,
-    sync::atomic::{AtomicU8, AtomicU16, Ordering},
+    sync::atomic::{AtomicU8, AtomicU16, AtomicUsize, Ordering},
 };
 use itertools::Itertools;
 use malachite_bigint::BigInt;
@@ -411,6 +411,10 @@ impl TryFrom<&[u8]> for CodeUnit {
 pub struct CodeUnits {
     units: UnsafeCell<Box<[CodeUnit]>>,
     adaptive_counters: Box<[AtomicU16]>,
+    /// Pointer-sized cache entries for descriptor pointers.
+    /// Single atomic load/store prevents torn reads when multiple threads
+    /// specialize the same instruction concurrently.
+    pointer_cache: Box<[AtomicUsize]>,
 }
 
 // SAFETY: All cache operations use atomic read/write instructions.
@@ -432,9 +436,15 @@ impl Clone for CodeUnits {
             .iter()
             .map(|c| AtomicU16::new(c.load(Ordering::Relaxed)))
             .collect();
+        let pointer_cache = self
+            .pointer_cache
+            .iter()
+            .map(|c| AtomicUsize::new(c.load(Ordering::Relaxed)))
+            .collect();
         Self {
             units: UnsafeCell::new(units),
             adaptive_counters,
+            pointer_cache,
         }
     }
 }
@@ -472,13 +482,19 @@ impl<const N: usize> From<[CodeUnit; N]> for CodeUnits {
 impl From<Vec<CodeUnit>> for CodeUnits {
     fn from(value: Vec<CodeUnit>) -> Self {
         let units = value.into_boxed_slice();
-        let adaptive_counters = (0..units.len())
+        let len = units.len();
+        let adaptive_counters = (0..len)
             .map(|_| AtomicU16::new(0))
             .collect::<Vec<_>>()
             .into_boxed_slice();
+        let pointer_cache = (0..len)
+            .map(|_| AtomicUsize::new(0))
+            .collect::<Vec<_>>()
+            .into_boxed_slice();
         Self {
             units: UnsafeCell::new(units),
             adaptive_counters,
+            pointer_cache,
         }
     }
 }
@@ -600,25 +616,25 @@ impl CodeUnits {
         lo | (hi << 16)
     }
 
-    /// Write a u64 value across four consecutive CACHE code units starting at `index`.
+    /// Store a pointer-sized value atomically in the pointer cache at `index`.
+    ///
+    /// Uses a single `AtomicUsize` store to prevent torn writes when
+    /// multiple threads specialize the same instruction concurrently.
     ///
     /// # Safety
-    /// Same requirements as `write_cache_u16`.
-    pub unsafe fn write_cache_u64(&self, index: usize, value: u64) {
-        unsafe {
-            self.write_cache_u32(index, value as u32);
-            self.write_cache_u32(index + 2, (value >> 32) as u32);
-        }
+    /// - `index` must be in bounds.
+    pub unsafe fn write_cache_ptr(&self, index: usize, value: usize) {
+        self.pointer_cache[index].store(value, Ordering::Relaxed);
     }
 
-    /// Read a u64 value from four consecutive CACHE code units starting at `index`.
+    /// Load a pointer-sized value atomically from the pointer cache at `index`.
+    ///
+    /// Uses a single `AtomicUsize` load to prevent torn reads.
     ///
     /// # Panics
-    /// Panics if `index + 3` is out of bounds.
-    pub fn read_cache_u64(&self, index: usize) -> u64 {
-        let lo = self.read_cache_u32(index) as u64;
-        let hi = self.read_cache_u32(index + 2) as u64;
-        lo | (hi << 32)
+    /// Panics if `index` is out of bounds.
+    pub fn read_cache_ptr(&self, index: usize) -> usize {
+        self.pointer_cache[index].load(Ordering::Relaxed)
     }
 
     /// Read adaptive counter bits for instruction at `index`.

diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs
@@ -64,17 +64,18 @@ static NEXT_TYPE_VERSION: AtomicU32 = AtomicU32::new(1);
 // Method cache (type_cache / MCACHE): direct-mapped cache keyed by
 // (tp_version_tag, interned_name_ptr).
 //
-// Uses a lock-free SeqLock pattern:
-//  - version acts as both cache key AND sequence counter
-//  - Read: load version (Acquire), read value ptr, re-check version
-//  - Write: set version=0 (invalidate), store value, store version (Release)
+// Uses a lock-free SeqLock pattern for the read/write protocol:
+//  - Readers validate sequence/version/name before and after the value read.
+//  - Writers bracket updates with sequence odd/even transitions.
 // No mutex needed on the hot path (cache hit).
 
 const TYPE_CACHE_SIZE_EXP: u32 = 12;
 const TYPE_CACHE_SIZE: usize = 1 << TYPE_CACHE_SIZE_EXP;
 const TYPE_CACHE_MASK: usize = TYPE_CACHE_SIZE - 1;
 
 struct TypeCacheEntry {
+    /// Sequence lock (odd = write in progress, even = quiescent).
+    sequence: AtomicU32,
     /// tp_version_tag at cache time. 0 = empty/invalid.
     version: AtomicU32,
     /// Interned attribute name pointer (pointer equality check).
@@ -94,12 +95,39 @@ unsafe impl Sync for TypeCacheEntry {}
 impl TypeCacheEntry {
     fn new() -> Self {
         Self {
+            sequence: AtomicU32::new(0),
             version: AtomicU32::new(0),
             name: AtomicPtr::new(core::ptr::null_mut()),
             value: AtomicPtr::new(core::ptr::null_mut()),
         }
     }
 
+    #[inline]
+    fn begin_write(&self) {
+        self.sequence.fetch_add(1, Ordering::AcqRel);
+    }
+
+    #[inline]
+    fn end_write(&self) {
+        self.sequence.fetch_add(1, Ordering::Release);
+    }
-    fn begin_write(&self) {
-        self.sequence.fetch_add(1, Ordering::AcqRel);
-    }
-
-    #[inline]
-    fn end_write(&self) {
-        self.sequence.fetch_add(1, Ordering::Release);
-    }
+    fn begin_write(&self) {
+        let mut seq = self.sequence.load(Ordering::Acquire);
+        loop {
+            while (seq & 1) != 0 {
+                core::hint::spin_loop();
+                seq = self.sequence.load(Ordering::Acquire);
+            }
+            match self.sequence.compare_exchange_weak(
+                seq,
+                seq.wrapping_add(1),
+                Ordering::AcqRel,
+                Ordering::Acquire,
+            ) {
+                Ok(_) => break,
+                Err(observed) => seq = observed,
+            }
+        }
+    }
+
+    #[inline]
+    fn end_write(&self) {
+        self.sequence.fetch_add(1, Ordering::Release);
+    }
-    fn begin_write(&self) {
-        self.sequence.fetch_add(1, Ordering::AcqRel);
-    }
-
-    #[inline]
-    fn end_write(&self) {
-        self.sequence.fetch_add(1, Ordering::Release);
-    }
+    fn begin_write(&self) {
+        let mut seq = self.sequence.load(Ordering::Acquire);
+        loop {
+            while (seq & 1) != 0 {
+                core::hint::spin_loop();
+                seq = self.sequence.load(Ordering::Acquire);
+            }
+            match self.sequence.compare_exchange_weak(
+                seq,
+                seq.wrapping_add(1),
+                Ordering::AcqRel,
+                Ordering::Acquire,
+            ) {
+                Ok(_) => break,
+                Err(observed) => seq = observed,
+            }
+        }
+    }
+
+    #[inline]
+    fn end_write(&self) {
+        self.sequence.fetch_add(1, Ordering::Release);
+    }
+
+    #[inline]
+    fn begin_read(&self) -> u32 {
+        let mut sequence = self.sequence.load(Ordering::Acquire);
+        while (sequence & 1) != 0 {
+            core::hint::spin_loop();
+            sequence = self.sequence.load(Ordering::Acquire);
+        }
+        sequence
+    }
+
+    #[inline]
+    fn end_read(&self, previous: u32) -> bool {
+        core::sync::atomic::fence(Ordering::Acquire);
+        self.sequence.load(Ordering::Relaxed) == previous
+    }
+
     /// Take the value out of this entry, returning the owned PyObjectRef.
     /// Caller must ensure no concurrent reads can observe this entry
     /// (version should be set to 0 first).
@@ -137,10 +165,14 @@ fn type_cache_clear_version(version: u32) {
     let mut to_drop = Vec::new();
     for entry in TYPE_CACHE.iter() {
         if entry.version.load(Ordering::Relaxed) == version {
-            entry.version.store(0, Ordering::Release);
-            if let Some(v) = entry.take_value() {
-                to_drop.push(v);
+            entry.begin_write();
+            if entry.version.load(Ordering::Relaxed) == version {
+                entry.version.store(0, Ordering::Release);
+                if let Some(v) = entry.take_value() {
+                    to_drop.push(v);
+                }
             }
+            entry.end_write();
         }
     }
     drop(to_drop);
@@ -158,10 +190,12 @@ pub fn type_cache_clear() {
     // Invalidate all entries and collect values.
     let mut to_drop = Vec::new();
     for entry in TYPE_CACHE.iter() {
+        entry.begin_write();
         entry.version.store(0, Ordering::Release);
         if let Some(v) = entry.take_value() {
             to_drop.push(v);
         }
+        entry.end_write();
     }
     drop(to_drop);
     TYPE_CACHE_CLEARING.store(false, Ordering::Release);
@@ -701,8 +735,11 @@ impl PyType {
     }
 
     pub fn set_attr(&self, attr_name: &'static PyStrInterned, value: PyObjectRef) {
-        self.attributes.write().insert(attr_name, value);
+        // Invalidate caches BEFORE modifying attributes so that cached
+        // descriptor pointers are still alive when type_cache_clear_version
+        // drops the cache's strong references.
         self.modified();
+        self.attributes.write().insert(attr_name, value);
     }
 
     /// Internal get_attr implementation for fast lookup on a class.
@@ -718,41 +755,44 @@ impl PyType {
     /// find_name_in_mro with method cache (MCACHE).
     /// Looks in tp_dict of types in MRO, bypasses descriptors.
     ///
-    /// Uses a lock-free SeqLock pattern keyed by version:
-    ///   Read:  load version → check name → load value → clone → re-check version
-    ///   Write: version=0 → swap value → set name → version=assigned
+    /// Uses a lock-free SeqLock-style pattern:
+    ///   Read:  load sequence/version/name → load value + try_to_owned →
+    ///          validate value pointer + sequence
+    ///   Write: sequence(begin) → version=0 → swap value/name → version=assigned → sequence(end)
     fn find_name_in_mro(&self, name: &'static PyStrInterned) -> Option<PyObjectRef> {
         let version = self.tp_version_tag.load(Ordering::Acquire);
         if version != 0 {
             let idx = type_cache_hash(version, name);
             let entry = &TYPE_CACHE[idx];
-            let v1 = entry.version.load(Ordering::Acquire);
-            if v1 == version
-                && core::ptr::eq(
-                    entry.name.load(Ordering::Relaxed),
-                    name as *const _ as *mut _,
-                )
-            {
+            let name_ptr = name as *const _ as *mut _;
+            loop {
+                let seq1 = entry.begin_read();
+                let v1 = entry.version.load(Ordering::Acquire);
+                let type_version = self.tp_version_tag.load(Ordering::Acquire);
+                if v1 != type_version
+                    || !core::ptr::eq(entry.name.load(Ordering::Relaxed), name_ptr)
+                {
+                    break;
+                }
                 let ptr = entry.value.load(Ordering::Acquire);
-                if !ptr.is_null() {
-                    // SAFETY: The value pointer was stored via PyObjectRef::into_raw
-                    // and is valid as long as the version hasn't changed. We create
-                    // a temporary reference (ManuallyDrop prevents decrement), clone
-                    // it to get our own strong reference, then re-check the version
-                    // to confirm the entry wasn't invalidated during our read.
-                    let cloned = unsafe {
-                        let tmp = core::mem::ManuallyDrop::new(PyObjectRef::from_raw(
-                            NonNull::new_unchecked(ptr),
-                        ));
-                        (*tmp).clone()
-                    };
-                    // SeqLock validation: if version changed, discard our clone
-                    let v2 = entry.version.load(Ordering::Acquire);
-                    if v2 == v1 {
+                if ptr.is_null() {
+                    if entry.end_read(seq1) {
+                        break;
+                    }
+                    continue;
+                }
+                // _Py_TryIncrefCompare-style validation:
+                // safe_inc, then ensure the source pointer is unchanged.
+                let obj: &PyObject = unsafe { &*ptr };
+                if let Some(cloned) = obj.try_to_owned() {
+                    let same_ptr = core::ptr::eq(entry.value.load(Ordering::Relaxed), ptr);
+                    if same_ptr && entry.end_read(seq1) {
                         return Some(cloned);
                     }
                     drop(cloned);
+                    continue;
                 }
+                break;
             }
         }
 
@@ -777,16 +817,17 @@ impl PyType {
         {
             let idx = type_cache_hash(assigned, name);
             let entry = &TYPE_CACHE[idx];
+            let name_ptr = name as *const _ as *mut _;
+            entry.begin_write();
             // Invalidate first to prevent readers from seeing partial state
             entry.version.store(0, Ordering::Release);
             // Swap in new value (refcount held by cache)
             let new_ptr = found.clone().into_raw().as_ptr();
             let old_ptr = entry.value.swap(new_ptr, Ordering::Relaxed);
-            entry
-                .name
-                .store(name as *const _ as *mut _, Ordering::Relaxed);
+            entry.name.store(name_ptr, Ordering::Relaxed);
             // Activate entry — Release ensures value/name writes are visible
             entry.version.store(assigned, Ordering::Release);
+            entry.end_write();
             // Drop previous occupant (its version was already invalidated)
             if !old_ptr.is_null() {
                 unsafe {
@@ -832,20 +873,24 @@ impl PyType {
         if version != 0 {
             let idx = type_cache_hash(version, name);
             let entry = &TYPE_CACHE[idx];
-            let v1 = entry.version.load(Ordering::Acquire);
-            if v1 == version
-                && core::ptr::eq(
-                    entry.name.load(Ordering::Relaxed),
-                    name as *const _ as *mut _,
-                )
-            {
+            let name_ptr = name as *const _ as *mut _;
+            loop {
+                let seq1 = entry.begin_read();
+                let v1 = entry.version.load(Ordering::Acquire);
+                let type_version = self.tp_version_tag.load(Ordering::Acquire);
+                if v1 != type_version
+                    || !core::ptr::eq(entry.name.load(Ordering::Relaxed), name_ptr)
+                {
+                    break;
+                }
                 let ptr = entry.value.load(Ordering::Acquire);
-                if !ptr.is_null() {
-                    let v2 = entry.version.load(Ordering::Acquire);
-                    if v2 == v1 {
+                if entry.end_read(seq1) {
+                    if !ptr.is_null() {
                         return true;
                     }
+                    break;
                 }
+                continue;
             }
         }
 
@@ -1498,8 +1543,8 @@ impl PyType {
             PySetterValue::Assign(ref val) => {
                 let key = identifier!(vm, __type_params__);
                 self.check_set_special_type_attr(key, vm)?;
-                self.attributes.write().insert(key, val.clone().into());
                 self.modified();
+                self.attributes.write().insert(key, val.clone().into());
             }
             PySetterValue::Delete => {
                 // For delete, we still need to check if the type is immutable
@@ -1510,8 +1555,8 @@ impl PyType {
                     )));
                 }
                 let key = identifier!(vm, __type_params__);
-                self.attributes.write().shift_remove(&key);
                 self.modified();
+                self.attributes.write().shift_remove(&key);
             }
         }
         Ok(())