AtomicOption should have Sync bound on its type argument.

[ammaraskar]

Currently AtomicOption implements Sync for any type. However, this should really be bounded by T: Sync, otherwise it allows for types that were never meant to be used across threads to be smuggled across boundaries through the use of an &AtomicOption. (Issue found by @Qwaz).

See this example of using AtomicOption to use a Cell across thread boundaries, leading to an arbitrary pointer dereference:

#![forbid(unsafe_code)]

use atomic_option::AtomicOption;

use crossbeam_utils::thread;
use std::{cell::Cell, sync::atomic::Ordering};

static SOME_INT: u64 = 123;

fn main() {
    #[derive(Debug, Clone, Copy)]
    enum RefOrInt<'a> {
        Ref(&'a u64),
        Int(u64),
    }
    let cell = Cell::new(RefOrInt::Ref(&SOME_INT));

    let atomic_opt = AtomicOption::new(Box::new(&cell));
    let ref_to_atomic_opt = &atomic_opt;

    thread::scope(|s| {
        s.spawn(move |_| {
            let cell_in_thread = *(ref_to_atomic_opt.take(Ordering::Relaxed).unwrap());
            println!("Thread - {:p} - {:?}", cell_in_thread, cell_in_thread);

            loop {
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                cell_in_thread.set(RefOrInt::Ref(&SOME_INT));
                cell_in_thread.set(RefOrInt::Int(0xdeadbeef));
            }
        });

        println!("Main - {:p} - {:?}", &cell, cell);
        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);
            }
        }
    });
}

Output:

Main - 0x7ffe55b08f58 - Cell { value: Ref(123) }
Thread - 0x7ffe55b08f58 - Cell { value: Ref(123) }
Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)

[JOE1994]

It seems to me that AtomicOption<T> not only needs T: Sync but also T: Send,
in order to prevent smuggling non-Send types like std::sync::MutexGuard<T> to other threads.

[yvt]

I don't think T: Sync is necessary because AtomicOption<T> doesn't let you borrow T while T is in AtomicOption - it just lets you move T between places. On the other hand, T: Send is undoubtedly necessary. (Actually, T: Send is enough for the example in the issue description to be rejected because in this case T is &Cell<_>, not Cell<_>.)