Slock<T> allows sending non-Send types across thread boundaries

[ammaraskar]

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

slock-rs/src/lib.rs

Lines 268 to 269 in 301f598

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

This should probably be bounded by T: Send just like the standard library's Mutex, otherwise this allows sending non-Send types across thread boundaries which may invoke undefined behavior.

Here's an example of this in action with an Rc segfaulting safe Rust code. Built with futures 0.3.8 with thread-pool feature enabled:

#![forbid(unsafe_code)]

use slock::Slock;

use std::rc::Rc;
use futures::executor::ThreadPool;

fn main() {
    let rc = Rc::new(());
    let lock = Slock::new(rc);
    let another_lock = lock.split();

    let fut1 = async move {
        let rc = lock.get_clone().await;
        println!("Future 1 - {:p}", rc);

        // Race the un-synchronized refcount in the RCs.
        for _ in 0..100_000_000 {
            rc.clone();
        }
    };
    let fut2 = async move {
        let rc = another_lock.get_clone().await;
        println!("Future 2 - {:p}", rc);

        for _ in 0..100_000_000 {
            rc.clone();
        }
    };

    let mut pool = ThreadPool::new().unwrap();
    pool.spawn_ok(fut1);
    pool.spawn_ok(fut2);
    // Give the pool time to complete its tasks.
    loop {}
}

This outputs:

Future 1 - 0x561c3cd6ba50
Future 2 - 0x561c3cd6ba50

Return Code: -4 (SIGILL)

[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.

[bakcxoj]

@ammaraskar @Shnatsel Thank you for the report!

This issue has been resolved.

[bakcxoj]

@ammaraskar This code now fails to compile because Rc does not implement Send:

#![forbid(unsafe_code)]

#[tokio::main]
async fn main() {
    let rc = Rc::new(());
    let lock = Slock::new(rc);
    let another_lock = lock.clone();

    let fut1 = async move {
        let rc = lock.get_clone().await;
        println!("Future 1 - {:p}", rc);

        // Race the un-synchronized refcount in the RCs.
        for _ in 0..100_000_000 {
            rc.clone();
        }
    };
    let fut2 = async move {
        let rc = another_lock.get_clone().await;
        println!("Future 2 - {:p}", rc);

        for _ in 0..100_000_000 {
            rc.clone();
        }
    };

    tokio::spawn(fut1);
    tokio::spawn(fut2);
}