SyncRef's clone() and debug() allow data races

[Qwaz]

Hello fellow Rustacean,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

v9/src/util.rs

Line 27 in 37756c6

unsafe impl<T> Sync for SyncRef<T> {}

SyncRef's Sync implementation doesn't impose Sync bound on T. This definition allows data races if &T is accessible through &SyncRef.

SyncRef derives Clone and Debug, and the default implementations of those traits access &T in such a way.

Reproduction

Below is an example program that exhibits undefined behavior using safe APIs of v9.

Show Detail

#![forbid(unsafe_code)]

use std::cell::Cell;
use std::fmt::Debug;
use std::thread;

use v9::util::SyncRef;

static STATIC_INT: u64 = 123;

// A simple tagged union for demonstrating the data race
#[derive(Clone, Copy)]
enum RefOrInt {
    Ref(&'static u64),
    Int(u128),
}

#[derive(Clone)]
struct RefOrIntCell(Cell<RefOrInt>);

impl RefOrIntCell {
    pub fn new() -> Self {
        RefOrIntCell(Cell::new(RefOrInt::Ref(&STATIC_INT)))
    }
}

impl Debug for RefOrIntCell {
    fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        loop {
            if let RefOrInt::Ref(addr) = self.0.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 == &STATIC_INT as *const u64 {
                    continue;
                }

                // We got an invalid reference with safe Rust code
                println!("Reference is pointing: {:p}", addr);
                println!("Dereferencing addr will segfault: {}", *addr);
            }
        }
    }
}

fn main() {
    // Creating &'static RefOrIntCell
    let ref_or_int = &*Box::leak(Box::new(RefOrIntCell::new()));
    // Creating &'static SyncRef<&'static RefOrIntCell>
    let sync_ref = &*Box::leak(Box::new(SyncRef::new(ref_or_int)));

    thread::spawn(move || {
        // Cell<_> is non-Sync, but `SyncRef` allows this type to be accessed concurrently by multiple threads
        format!("{:?}", sync_ref);
    });

    loop {
        // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
        ref_or_int.0.set(RefOrInt::Ref(&STATIC_INT));
        ref_or_int.0.set(RefOrInt::Int(0xdeadbeef));
    }
}

Output:

Reference is pointing: 0xdeadbeef

Terminated with signal 11 (SIGSEGV)

Tested Environment

• Crate: v9
• Version: 0.1.41
• OS: Ubuntu 20.04.1 LTS
• Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)

[Shnatsel]

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.

[purpleposeidon]

....oh, hello. Sorry, I'm going to be quite slow on this.

I should be able to fix this, but I believe there are other soundness holes in the crate.

[Shnatsel]

Crates with soundness holes are fine as long as they're explicitly labeled as such. If you put something like this in the README, it should clear up any confusion:

This crate is experimental and likely unsound. Don't use it in production just yet!