PinSlab<T> and Unordered<T, S> need bounds on their Send/Sync traits

[ammaraskar]

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that the PinSlab:

unicycle/src/pin_slab.rs

Lines 43 to 44 in f5a2838

	unsafe impl<T> Send for PinSlab<T> {}
	unsafe impl<T> Sync for PinSlab<T> {}

and Unordered

unicycle/src/lib.rs

Lines 380 to 381 in f5a2838

	unsafe impl<T, S> Send for Unordered<T, S> where S: Sentinel {}
	unsafe impl<T, S> Sync for Unordered<T, S> where S: Sentinel {}

types implement the Send and Sync traits for all types. This should likely be bounded by Send/Sync on the contained types otherwise it can lead to data-races from safe Rust code. Here's an example of such a data race with PinSlab:

#![forbid(unsafe_code)]

use unicycle::pin_slab::PinSlab;

use std::cell::Cell;
use crossbeam_utils::thread;

// A simple tagged union used to demonstrate problems with data races in Cell.
#[derive(Debug, Clone, Copy)]
enum RefOrInt { Ref(&'static u64), Int(u64) }
static SOME_INT: u64 = 123;

fn main() {
    let cell = Cell::new(RefOrInt::Ref(&SOME_INT));

    let mut slab = PinSlab::new();
    slab.insert(&cell);

    thread::scope(|s| {
        s.spawn(move |_| {
            loop {
                let smuggled_cell = slab.get(0).unwrap();
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                smuggled_cell.set(RefOrInt::Ref(&SOME_INT));
                smuggled_cell.set(RefOrInt::Int(0xdeadbeef));
            }
        });

        loop {
            if let RefOrInt::Ref(addr) = cell.get() {
                // Hope that between the time we pattern match the object as a
                // `Ref`, it gets written to by the other thread.
                if addr as *const u64 == &SOME_INT as *const u64 { continue; }

                // Due to the data race, obtaining Ref(0xdeadbeef) is possible
                println!("Pointer is now: {:p}", addr);
                println!("Dereferencing addr will now segfault: {}", *addr);
            }
        }
    });
}

which outputs:

Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)

[udoprog]

That is indeed a bug that will be fixed. Much obliged!

[udoprog]

Released in 0.7.0. Thanks again!

[ammaraskar]

Thank you for the quick response :)

[JOE1994]

@udoprog It seems that there was a small mistake while fixing this issue in 92f40b4.

unsafe impl<T, S> Send for Unordered<T, S> where S: Send + Sentinel {}
unsafe impl<T, S> Sync for Unordered<T, S> where S: Sync + Sentinel {}

Instead of adding trait bounds for S in Unordered<T, S>, I think a proper fix was to add trait bounds T: Send (to the Send impl) & T: Sync (to the Sync impl). S in Unordered<T, S> is only used within PhantomData<_> (dummy type) while T is the actual data contained within Unordered<T, S>.

[udoprog]

Instead of adding trait bounds for S in Unordered<T, S>, I think a proper fix was to add trait bounds T: Send (to the Send impl) & T: Sync (to the Sync impl). S in Unordered<T, S> is only used within PhantomData<_> (dummy type) while T is the actual data contained within Unordered<T, S>.

Yeah. That does indeed look wrong! I've seen that you've forked the project, so feel free if you want to send a fix.