Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

AtomicBox should have bounds on its Send/Sync traits #1

Closed
ammaraskar opened this issue Nov 10, 2020 · 1 comment · Fixed by #2
Closed

AtomicBox should have bounds on its Send/Sync traits #1

ammaraskar opened this issue Nov 10, 2020 · 1 comment · Fixed by #2

Comments

@ammaraskar
Copy link

ammaraskar commented Nov 10, 2020

Currently AtomicBox implements the Sync/Send traits unconditionally:

abox/src/lib.rs

Lines 92 to 93 in 5abe752

unsafe impl<T: Sized> Sync for AtomicBox<T> {}
unsafe impl<T: Sized> Send for AtomicBox<T> {}

I think this should only be when T: Sized + Sync and T: Sized + Send respectively. Otherwise, this makes it possible to send across types that aren't safe to use across threads such as Cells. Here's a demonstration that causes a data-race:

#![forbid(unsafe_code)]

use abox::AtomicBox;

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

#[derive(Debug, Clone, Copy)]
enum RefOrInt<'a> { Ref(&'a u64), Int(u64) }
static SOME_INT: u64 = 123;

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

    thread::scope(|s| {
        s.spawn(move |_| {
            let smuggled_cell = atomic_box.get();

            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) = 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)

@SonicFrog
Copy link
Owner

Thanks for the heads-up! I'll fix this issue as soon as I have a moment.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants