model icon indicating copy to clipboard operation
model copied to clipboard

Possible soundness issue in Shared?

Open ammaraskar opened this issue 5 years ago • 1 comments

Hi there, we (the Rust group at @sslab-gatech) are auditing crates on crates.io for safety issues and noticed that the Shared object seems to not have bounds on its Send/Sync traits:

https://github.com/spacejam/model/blob/b50d9eef17a80297a45772af4aa8a7413aa66c2c/src/lib.rs#L110-L112

We weren't sure if this was intentional since this is a testing library but this can be used to create data races from safe Rust by sharing types like Cell:

#![forbid(unsafe_code)]

use model::Shared;

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 shared = Shared::new(&cell);

    thread::scope(|s| {
        s.spawn(move |_| {
            loop {
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                shared.set(RefOrInt::Ref(&SOME_INT));
                shared.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);
            }
        }
    });
}

which outputs:

Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)

ammaraskar avatar Nov 10 '20 19:11 ammaraskar

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.

Since the project is intended for testing rather than production environments, it is surfaced as a warning rather than an error.

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.

Shnatsel avatar Jan 30 '21 18:01 Shnatsel