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 #15

Closed
ammaraskar opened this issue Nov 10, 2020 · 5 comments
Closed

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

ammaraskar opened this issue Nov 10, 2020 · 5 comments

Comments

@ammaraskar
Copy link

ammaraskar commented Nov 10, 2020

Currently AtomicBox implements the Sync/Send traits unconditionally:

lever/src/sync/atomics.rs

Lines 104 to 105 in 15a96c7

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 lever::sync::atomics::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)

@Shnatsel
Copy link

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

@o0Ignition0o
Copy link
Contributor

Hey! thanks for opening the issue.

You're correct! I think @vertexclique has a patch and we can roll it out hopefully soon

@o0Ignition0o
Copy link
Contributor

Added a regression test thanks to trybuild, super nice crate, i didn't know about it!

@o0Ignition0o
Copy link
Contributor

@vertexclique can we close this now ? :) I'm going to submit a aptch to the rustsec advisory database

@vertexclique
Copy link
Owner

Truly, let's close it.

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

No branches or pull requests

4 participants