Intern<T>: Data race allowed on T

[JOE1994]

Hello,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

Intern<T> unconditionally implements Sync. This allows users to create data races on T: !Sync.
Such data races can lead to undefined behavior.

internment/src/lib.rs

Line 150 in 863dbc0

unsafe impl<T> Sync for Intern<T> {}

Reproduction

Below is an example program that exhibits undefined behavior (memory corruption) using safe APIs of internment.

Show Detail

#![forbid(unsafe_code)]
use internment::Intern;

use std::borrow::Borrow;
use std::cell::Cell;
use std::hash::{Hash, Hasher};
use std::sync::Arc;

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

#[derive(Debug, PartialEq, Eq, Clone)]
struct Foo(Cell<RefOrInt>);

impl Hash for Foo {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.0.get().hash(state);
    }
}

impl Foo {
    fn set(&self, v: RefOrInt) {
        self.0.set(v);
    }
    fn get(&self) -> RefOrInt {
        self.0.get()
    }
}

fn main() {
    let non_sync = Foo(Cell::new(RefOrInt::Ref(&SOME_INT)));
    let i0 = Arc::new(Intern::new(non_sync));

    let i1 = i0.clone();
    std::thread::spawn(move || {
        let i1 = i1;

        loop {
            // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
            i1.set(RefOrInt::Ref(&SOME_INT));
            i1.set(RefOrInt::Int(0xdeadbeef));
        }
    });
    
    loop {
        if let RefOrInt::Ref(addr) = i0.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;
            }

            println!("Pointer is now: {:p}", addr);
            println!("Dereferencing addr will now segfault: {}", *addr);
        }
    }
}

Output:

Pointer is now: 0xdeadbeef

Terminated with signal 11 (SIGSEGV)

Tested Environment

• Crate: internment
• Version: 0.3.13
• OS: Ubuntu 18.04.5 LTS
• Rustc version: rustc 1.50.0 (cb75ad5db 2021-02-10)

[droundy]

Thanks for the report. Would the proper fix be to implement Sync only if T implements Send? Our should T: Sync be required?

[JOE1994]

I think 2928a87 was a proper fix for the issue.

Thank you for the quick fix and the new release on crates.io 🚀