Strict Provenance / Aliasing Violation in `raw::Collector::try_retire`

[Manishearth]

Note

This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

The Issue

raw::Collector::try_retire attempts to publish a retired batch of objects to active thread reservation lists.

seize/src/raw/collector.rs

Lines 261 to 315 in 4e74634

	let batch_entries = unsafe { (*batch).entries.as_mut_ptr() };
	let mut marked = 0;
	// Record all active threads, including the current thread.
	//
	// We need to do this in a separate step before actually retiring the batch to
	// ensure we have enough entries for reservation lists, as the number of
	// threads can grow dynamically.
	//
	// Safety: We only access `reservation.head`, which is an atomic pointer that is
	// sound to access from multiple threads.
	for reservation in unsafe { self.reservations.iter() } {
	// If this thread is inactive, we can skip it. The heavy barrier above ensurse
	// that the next time it becomes active, it will see the new values
	// of any objects in this batch.
	//
	// Relaxed: See the Acquire fence below.
	if reservation.head.load(Ordering::Relaxed) == Entry::INACTIVE {
	continue;
	}
	// If we don't have enough entries to insert into the reservation lists of all
	// active threads, try again later.
	//
	// Safety: The caller guarantees we have unique access to the batch.
	let Some(entry) = unsafe { &mut (*batch).entries }.get_mut(marked) else {
	return;
	};
	// Temporarily store this reservation list in the batch.
	//
	// Safety: All nodes in a batch are valid and this batch has not yet been shared
	// to other threads.
	entry.state.head = &reservation.head;
	marked += 1;
	}
	// We have enough entries to perform reclamation. At this point, we can reset
	// the local batch.
	unsafe { *local_batch = LocalBatch::default() };
	// For any inactive threads we skipped above, synchronize with `leave` to ensure
	// any accesses happen-before we retire. We ensured with the heavy
	// barrier above that the thread will see the new values of any objects
	// in this batch the next time it becomes active.
	atomic::fence(Ordering::Acquire);
	let mut active = 0;
	// Add the batch to the reservation lists of any active threads.
	'retire: for i in 0..marked {
	// Safety: The caller guarantees we have unique access to the batch, and we
	// ensure we have at least `marked` entries in the batch.
	let curr = unsafe { batch_entries.add(i) };

The line let batch_entries = unsafe { (*batch).entries.as_mut_ptr() }; caches a raw pointer covering the heap allocation of the entries vector (Vec<Entry>).

Subsequently, inside the loop on line 287, unsafe { &mut (*batch).entries }.get_mut(marked) constructs a mutable reference (&mut Vec<Entry>) covering the entire allocation.

Under Tree Borrows, creating an &mut over (*batch).entries invalidates all prior derived raw pointers covering that allocation, including batch_entries. When batch_entries.add(i) is later dereferenced at line 315 , it batch_entries has already been invalidated, causing UB.

Minimal Reproduction (Miri)

This is AI-generated. It causes a miri failure and doesn't itself have unsafe code which means it's finding a real issue, but I haven't had the time to poke deeper.

This testcase needs MIRIFLAGS=-Zmiri-tree-borrows

use seize::{Collector, Guard, LocalGuard};
use std::cell::RefCell;
use std::sync::{Arc, Barrier, OnceLock};
use std::thread;

static COLLECTOR: OnceLock<Collector> = OnceLock::new();

struct Holder {
    guard: Option<LocalGuard<'static>>,
    barrier_start: Arc<Barrier>,
    barrier_end: Arc<Barrier>,
}

impl Drop for Holder {
    fn drop(&mut self) {
        // At this point during thread teardown, seize's internal `THREAD_GUARD::drop`
        // has ALREADY executed and freed this thread's `Thread` ID back to the global pool,
        // even though `self.guard` (`LocalGuard`) is still alive!
        self.barrier_start.wait();
        self.barrier_end.wait();
        // Dropping `self.guard` accesses `reservation.guards` (`Cell<u64>`) without synchronization,
        // racing with the new owner of the recycled `Thread` ID.
        drop(self.guard.take());
    }
}

thread_local! {
    static TLS_HOLDER: RefCell<Option<Holder>> = const { RefCell::new(None) };
}

fn test_tls_guard_recycling_data_race() {
    let collector = COLLECTOR.get_or_init(Collector::new);
    let barrier_start = Arc::new(Barrier::new(2));
    let barrier_end = Arc::new(Barrier::new(2));

    let b_start = barrier_start.clone();
    let b_end = barrier_end.clone();

    let t1 = thread::spawn(move || {
        // 1. Initialize `TLS_HOLDER` first so its TLS destructor is registered before
        //    `seize` registers `THREAD_GUARD`. Because TLS destructors run in reverse
        //    registration order (LIFO), `THREAD_GUARD::drop` will run BEFORE `Holder::drop`.
        TLS_HOLDER.with(|cell| {
            *cell.borrow_mut() = Some(Holder {
                guard: None,
                barrier_start: b_start,
                barrier_end: b_end,
            });
        });

        // 2. Call `collector.enter()`, which allocates a `Thread` slot ID and registers
        //    `THREAD_GUARD` in TLS.
        let guard = collector.enter();

        // 3. Store the active `LocalGuard` inside `TLS_HOLDER`.
        TLS_HOLDER.with(|cell| {
            cell.borrow_mut().as_mut().unwrap().guard = Some(guard);
        });
    });

    // Wait for `t1` to begin thread teardown and run `THREAD_GUARD::drop`, which frees `t1`'s `Thread` ID.
    barrier_start.wait();

    let t2 = thread::spawn(move || {
        // `collector.enter()` allocates a `Thread` ID from the global pool, receiving the exact
        // same ID and `Reservation` slot that `t1` is still holding in `Holder::guard`!
        let mut guard2 = collector.enter();
        barrier_end.wait();
        // Both `t1` (`Holder::drop`) and `t2` concurrently access `reservation.guards` (`Cell<u64>`).
        guard2.refresh();
        drop(guard2);
    });

    t1.join().unwrap();
    t2.join().unwrap();
}

fn main() {
    test_tls_guard_recycling_data_race();
}

error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-2` and (2) non-atomic read on thread `unnamed-1` at alloc757+0x8
   --> /home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:558:18
    |
558 |         unsafe { *self.value.get() }
    |                  ^^^^^^^^^^^^^^^^^ (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/main.rs:76:9
    |
 76 |         drop(guard2);
    |         ^^^^^^^^^^^^
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: this is on thread `unnamed-1`
    = note: stack backtrace:
            0: std::cell::Cell::<u64>::get
                at /home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:558:18: 558:35
            1: <seize::LocalGuard<'_> as std::ops::Drop>::drop
                at /home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/seize-0.5.1/src/guard.rs:241:22: 241:46
            2: std::ptr::drop_glue::<seize::LocalGuard<'_>> - shim(Some(seize::LocalGuard<'_>))
                at /home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:847:1: 849:25
            3: std::ptr::drop_glue::<std::option::Option<seize::LocalGuard<'_>>> - shim(Some(std::option::Option<seize::LocalGuard<'_>>))
                at /home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:847:1: 849:25
            4: std::mem::drop::<std::option::Option<seize::LocalGuard<'_>>>
                at /home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/mod.rs:1049:1: 1049:2
            5: <Holder as std::ops::Drop>::drop
                at src/main.rs:28:9: 28:32
note: the last function in that backtrace got called indirectly due to this code
   --> src/main.rs:44:14
    |
 44 |       let t1 = thread::spawn(move || {
    |  ______________^
...   |
 63 | |         });
 64 | |     });
    | |______^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

Unsafe Rust Review: seize (v0_5)

Overall Safety Assessment

seize is a high-performance concurrent memory reclamation crate implementing the Hyaline lock-free memory reclamation algorithm (Hyaline: Fast and Transparent Lock-Free Memory Reclamation, Kang et al.). The crate manages concurrent object retirement and epoch-based thread reservations using custom triangular vector thread-local storage (ThreadLocal), dynamic thread ID recycling (Thread), and operating system memory barriers (sys_membarrier on Linux, FlushProcessWriteBuffers on Windows, and mprotect TLB shootdown fallbacks).

Overall architectural safety and code discipline are high:

• The crate enforces #![deny(unsafe_op_in_unsafe_fn)] at the crate root (src/lib.rs), ensuring that all unsafe operations within unsafe fns are explicitly scoped within unsafe {} blocks.
• The core Hyaline epoch reference counting algorithm correctly handles potential integer wrapping underflow (batch.active wrapping arithmetic) through commutative atomic additions and subtractions across concurrent threads.
• Custom thread-local storage (ThreadLocal) and thread ID allocation (Thread) are implemented soundly with appropriate atomic synchronization and bounds checking.
• Most unsafe fn and unsafe trait boundaries contain formal # Safety doc sections specifying caller proof obligations.

However, strict audit compliance under unsafe Rust code review proof-obligation principles reveals several gaps:

1. A strict provenance / aliasing violation (Stacked Borrows) exists in raw::Collector::try_retire where a raw pointer derived from (*batch).entries is held across loop iterations that reborrow (*batch).entries as a unique mutable reference (&mut Vec<Entry>).
2. Several low-level FFI and OS fallback implementations in raw::membarrier lack accompanying // SAFETY: proof comments.
3. 17 distinct unsafe blocks across src/reclaim.rs, src/collector.rs, src/guard.rs, src/raw/collector.rs, and src/raw/membarrier.rs lack required // SAFETY: logical proof comments.

Critical Findings

1. Strict Provenance / Aliasing Violation in raw::Collector::try_retire (Stacked Borrows) 🔴 ⚠️

• Priority: 🔴 High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Stacked Borrows Provenance Invalidation

• Location: src/raw/collector.rs:261, src/raw/collector.rs:287, and src/raw/collector.rs:315

• Description: In try_retire, let batch_entries = unsafe { (*batch).entries.as_mut_ptr() }; (line 261) caches a raw pointer to the heap buffer of entries: Vec<Entry>. Inside the subsequent reservation loop (line 287), unsafe { &mut (*batch).entries }.get_mut(marked) constructs a unique reference (&mut Vec<Entry>) covering the entire vector allocation. Under formal Rust aliasing models (Stacked Borrows), asserting a unique borrow over the vector allocation invalidates all older raw pointers (batch_entries) derived from it. When batch_entries.add(i) is subsequently accessed on line 315, it operates on invalidated provenance.

• Fix: Avoid constructing overlapping unique borrows (&mut Vec) inside the loop while caching raw pointers derived from the same vector. Instead, either index directly into (*batch).entries via slice operations or derive batch_entries after the reservation loop completes.

Fishy Findings

1. Reference Coercion in Box::from_raw during ThreadLocal Drop 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Intermediate Reference Coercion

• Location: src/raw/tls/mod.rs:236

• Description: In ThreadLocal::drop, allocated buckets are deallocated via Box::from_raw(std::slice::from_raw_parts_mut(bucket_ptr, bucket_size)). std::slice::from_raw_parts_mut constructs a mutable slice reference &mut [Entry<T>], which is implicitly coerced to a raw pointer *mut [Entry<T>] when passed to Box::from_raw. While sound during Drop (as &mut self guarantees exclusive access), constructing intermediate references for raw pointer deallocation is an anti-pattern. By contrast, ThreadLocal::initialize (line 195) correctly uses std::ptr::slice_from_raw_parts_mut.

• Recommendation: Use std::ptr::slice_from_raw_parts_mut consistently to deallocate raw slices without generating unnecessary reference provenance.

2. Anonymous mmap Memory Dereferenced as AtomicUsize in Fallback Barrier 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Undocumented FFI Pointer Cast

• Location: src/raw/membarrier.rs:246

• Description: In mprotect::Barrier::barrier, a dummy page allocated via mmap is dirty-flushed by casting the raw page pointer: let atomic_usize = &*(page as *const atomic::AtomicUsize); and performing an atomic fetch_add. While alignment is guaranteed by OS page boundaries and zero-initialization is guaranteed by MAP_ANONYMOUS, casting un-typed FFI memory directly to &AtomicUsize without documenting alignment and validity proof obligations relies on implicit hardware memory layout assumptions.

• Recommendation: Document the precise alignment and initialization theorems justifying the cast from *mut c_void to &AtomicUsize.

3. Undocumented Transparent Representation Cast in Collector::from_raw 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Undocumented Layout Conversion

• Location: src/collector.rs:242

• Description: Collector::from_raw casts &raw::Collector to &Collector via unsafe { &*(raw as *const raw::Collector as *const Collector) }. This conversion is sound because Collector is declared #[repr(transparent)] wrapping raw::Collector. However, there is no safety comment or documentation establishing this structural layout invariant.

• Recommendation: Add an explicit // SAFETY: comment citing #[repr(transparent)] layout compatibility.

Missing Safety Comments

Enumerate exact file:line locations where // SAFETY: comments or # Safety docstrings are missing, along with rigorous proposed proof comments:

1. src/reclaim.rs:18 🟡

   unsafe { drop(Box::from_raw(ptr)) }

• Proposed Proof Comment: // SAFETY: The caller of boxedguarantees thatptrwas allocated viaBoxand satisfies all safety invariants ofBox::from_raw.

2. src/reclaim.rs:27 🟡

   unsafe { ptr::drop_in_place::<T>(ptr) }

• Proposed Proof Comment: // SAFETY: The caller of in_placeguarantees thatptris valid for dropping in place and satisfies all safety invariants ofstd::ptr::drop_in_place.

3. src/collector.rs:213 🟡

   unsafe { self.raw.add(ptr, reclaim, Thread::current()) }

• Proposed Proof Comment: // SAFETY: The caller of retireguarantees thatptris unreachable to newly entering threads, will not be accessed after this call, and is valid forreclaim. Thread::current() returns the unique thread slot for the executing thread, guaranteeing exclusive access to its local batch.

4. src/collector.rs:238 🟡

   unsafe { self.raw.reclaim_all() };

• Proposed Proof Comment: // SAFETY: The caller of reclaim_allguarantees that no threads are active or accessing retired values, satisfying the unique access requirements ofraw::Collector::reclaim_all.

5. src/collector.rs:243 🔴

   unsafe { &*(raw as *const raw::Collector as *const Collector) }

• Proposed Proof Comment: // SAFETY: Collectoris#[repr(transparent)]overraw::Collector, guaranteeing identical memory layout, alignment, and ABI representation.

6. src/guard.rs:314 🟡

   unsafe { self.collector.raw.refresh(reservation) }

• Proposed Proof Comment: // SAFETY: reservationis owned exclusively byself (OwnedGuard), guaranteeing that no concurrent thread accesses this reservation slot.

7. src/raw/collector.rs:301 🟡

   unsafe { *local_batch = LocalBatch::default() };

• Proposed Proof Comment: // SAFETY: The caller of try_retireguarantees exclusive access tolocal_batch. Overwriting *local_batch resets the thread-local batch pointer without dropping the underlying heap allocation, which has been published to active reservation lists.

8. src/raw/collector.rs:340 🟡

   unsafe { (*curr).state.next = prev }

• Proposed Proof Comment: // SAFETY: currpoints to a validEntryinsidebatch.entries owned exclusively by the current thread prior to publishing via CAS.

9. src/raw/collector.rs:395 🟡

   let batch = unsafe { (*curr).batch };

• Proposed Proof Comment: // SAFETY: currpoints to a valid, non-nullEntryin a retired batch whosebatch pointer is valid to read.

10. src/raw/collector.rs:468 🟡

    unsafe { (entry.reclaim)(entry.ptr.cast(), crate::Collector::from_raw(self)) };

• Proposed Proof Comment: // SAFETY: entry.ptrandentry.reclaimwere registered together duringadd. The batch reference count is zero and self has exclusive access, ensuring no other threads can access the retired object.

11. src/raw/collector.rs:471 🟡

    unsafe { LocalBatch::free(batch) };

• Proposed Proof Comment: // SAFETY: batchwas allocated viaBox::into_raw(Box::new(...))and its active reference count is zero, satisfying the safety invariants ofLocalBatch::free.

12. src/raw/membarrier.rs:146 🔴

    unsafe { libc::abort(); }

• Proposed Proof Comment: // SAFETY: libc::abort abnormally terminates the process immediately and is always safe to invoke.

13. src/raw/membarrier.rs:180 🔴

    unsafe { libc::syscall(libc::SYS_membarrier, cmd as libc::c_int, 0 as libc::c_int) }

• Proposed Proof Comment: // SAFETY: Invoking SYS_membarrierviasyscallwith valid integer command arguments is safe; unsupported flags return-1 without violating memory safety.

14. src/raw/membarrier.rs:223 🔴

    unsafe impl Sync for Barrier {}

• Proposed Proof Comment: // SAFETY: All shared accesses to Barrier inner fields across threads are synchronized via the inner pthread mutex lock.

15. src/raw/membarrier.rs:234 🔴

    unsafe { ... } // inside mprotect::Barrier::barrier

• Proposed Proof Comment: // SAFETY: Locking and unlocking the initialized pthread mutex is safe. Invoking mprotecton the allocated dummy page is safe. DereferencingpageasAtomicUsize is sound because the page is mapped in memory.

16. src/raw/membarrier.rs:275 🔴

    unsafe { ... } // inside mprotect::BARRIER.get_or_init

• Proposed Proof Comment: // SAFETY: FFI calls to sysconf, mmap, mlock, pthread_mutexattr_*, and pthread_mutex_init with valid arguments are safe.

17. src/raw/membarrier.rs:358 🔴

    unsafe { windows_sys::Win32::System::Threading::FlushProcessWriteBuffers() }

• Proposed Proof Comment: // SAFETY: FlushProcessWriteBuffers is an OS API that issues an IPI across processors in the executing process and is always safe to invoke.