OnceCell can send (!Send && Sync) objects across thread boundaries

[JOE1994]

Hello 🦀 ,
we (Rust group @sslab-gatech) found an undefined-behavior issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Sync impl of OnceCell

By exploiting the fact that T has no Send bound,
it is possible to make OnceCell send a non-Send object across thread boundaries.

unsafe impl<T, B> Sync for OnceCell<T, B> where T: Sync {}

Proof of Concept

I prepared a small example that sends std::sync::MutexGuard across thread boundaries using OnceCell.

use conquer_once::OnceCell;
use crossbeam_utils::thread;

use std::sync::Mutex;

fn main() {
    let once_cell = OnceCell::uninit();  
    thread::scope(|s| {
        s.spawn(|_| {
            once_cell.try_init_once(move || {
                let mutex_static = Box::leak(Box::new(Mutex::new(0_i32)));

                // `MutexGuard`is `Sync`, but not `Send`.
                let mutex_guard = mutex_static.lock().unwrap();
                let tid = std::thread::current().id();
                (mutex_guard, tid)
            }).unwrap();
        });
    }).unwrap();

    if let Some((smuggled_mutexguard, tid)) = once_cell.into_inner() {
        // `smuggled_mutexguard` is dropped at the end of its lexical scope.
        // The parent thread attempt to unlock the Mutex which it did not lock.
        // 
        // If a thread attempts to unlock a Mutex that it has not locked, it can result in undefined behavior.
        // (https://github.com/rust-lang/rust/issues/23465#issuecomment-82730326)
        assert_eq!(tid, std::thread::current().id());
    }
}

Suggested Fix

Adding a Send bound to T as following will allow the compiler to revoke the example program above.

unsafe impl<T, B> Sync for OnceCell<T, B> where T: Send + Sync {}

Thank you for checking out this issue 👍

[oliver-giersch]

Thank you very much for bringing this issue up!