Skip to content
New issue

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

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

Already on GitHub? Sign in 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.

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

Successfully merging a pull request may close this issue.

2 participants