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

convec::ConVec lacks a bound on its Send trait and Sync trait #2

Open
setuid0x0 opened this issue Nov 24, 2020 · 1 comment
Open

convec::ConVec lacks a bound on its Send trait and Sync trait #2

setuid0x0 opened this issue Nov 24, 2020 · 1 comment

Comments

@setuid0x0
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs.
We noticed that the convec::ConVec object implements the Send and Sync traits for all types:

convec/src/convec.rs

Lines 16 to 17 in 1591dcd

unsafe impl<T> Sync for ConVec<T> {}
unsafe impl<T> Send for ConVec<T> {}

This allows objects like Cell that doesn't implement Sync to be shared across threads leading to undefined behavior.
The code below crashes due to the data race.

#![forbid(unsafe_code)]

use std::cell::Cell;

use convec::*;
use crossbeam_utils::thread;

static SOME_INT: u128 = 0x41414141;

fn main() {
    // A simple tagged union used to demonstrate the problems with data races
    // in Cell. Cell is designed for single threads and has no synchronization
    // methods. Thus if it is allowed to be used simultaneously by two threads,
    // it is possible to race its interior mutability methods to dereference an
    // arbitrary pointer.
    #[derive(Debug, Clone, Copy)]
    enum RefOrInt<'a> {
        Ref(&'a u128),
        Int(u128),
    }

    let mut vec: AoVec<Cell<RefOrInt>> = AoVec::new();
    vec.push(Cell::new(RefOrInt::Ref(&SOME_INT)));

    thread::scope(|s| {
        let vec_ref = &vec;
        let child = s.spawn(move |_| {
            let smuggled = vec_ref.get(0).unwrap();

            println!("Child thread: {:p} - {:?}", smuggled.as_ptr(), smuggled);
            loop {
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                smuggled.set(RefOrInt::Ref(&SOME_INT));
                smuggled.set(RefOrInt::Int(0xdeadbeef));
            }
        });

        let main_cell = vec_ref.get(0).unwrap();
        println!("Main thread: {:p} - {:?}", main_cell.as_ptr(), main_cell);
        loop {
            if let RefOrInt::Ref(addr) = main_cell.clone().into_inner() {
                // Hope that between the time we pattern match the object as a
                // `Ref`, it gets written to by the child thread.
                if addr as *const u128 == &SOME_INT as *const u128 {
                    continue;
                }

                // Due to the data race, obtaining Ref(0xdeadbeef) is possible
                println!("Reference points to: {:p}", addr);
                println!("Dereferencing addr will now segfault: {}", *addr);
            }
        }
    });
}
JOE1994 added a commit to JOE1994/convec that referenced this issue Jan 23, 2021
JOE1994 added a commit to JOE1994/convec that referenced this issue Jan 23, 2021
@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.

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

2 participants