HConsed's Send and Sync traits should be bounded on the contained type

[ammaraskar]

Currently, HConsed implements the Send and Sync traits unconditionally:

hashconsing/src/lib.rs

Lines 354 to 355 in 1b91c14

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

This is a possible soundness issue because it allows types T that aren't necessarily thread safe to be used across threads as long as they are wrapped in an HConsed<T>.

Sort of a contrived example but the following demonstrates a data-race that segfaults safe rust using this:

Click to expand example

#![forbid(unsafe_code)]

use hashconsing::{HConsign, HConsed, HashConsign};

use std::hash::{Hash, Hasher};
use std::cell::Cell;
use crossbeam_utils::thread;

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
enum RefOrInt<'a> {
    Ref(&'a u64),
    Int(u64),
}

#[derive(PartialEq, Eq)]
struct HashableCell<T: Eq + PartialEq + Copy> { cell: Cell<T> }
// Fake hashing function just so we can get a HConsed going.
impl<T: Eq + PartialEq + Copy> Hash for HashableCell<T> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        1024.hash(state);
    }
}

static SOME_INT: u64 = 123;

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

    let mut factory: HConsign<_> = HConsign::empty();
    let hcons_cell_ref = factory.mk(&hashable_cell);
    thread::scope(|s| {
        s.spawn(move |_| {
            let smuggled_cell = &hcons_cell_ref.get().cell;

            loop {
                // 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) = hashable_cell.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);
            }
        }
    });
}

This outputs:

Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)

(Issue found by @sslab-gatech's Rust group)

[AdrienChampion]

Wow. I wrote this a while ago, I think these two lines are leftovers from a previous version. Very bad unsafe leftovers.

I just removed the implementations and

• added your example as a test that should not compile here, output here
• added a test making sure HConsed has Send and Sync when it should here

Does this look fine to you?
Thank you for your feedback by the way :D

[ammaraskar]

Thank you for the quick response, this looks good to me 🙂

[AdrienChampion]

Awesome!

I just updated the official crates.io version.