ButtplugFutureStateShared allows data race to (!Send|!Sync) objects

[JOE1994]

Hello 🦀,
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 (Describe the bug)

Send/Sync are unconditionally implemented for ButtplugFutureStateShared,
and thus it is possible to cause a data race to a (!Send|!Sync) object.

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

Proof of Concept (Actual behavior)

Below is an example program that segfaults while using ButtplugFutureStateShared.
Segmentation fault was observed when the program was built with rustc 1.49.0-nightly in Debug mode (on Ubuntu 18.04).
The program below allows two threads to concurrently access the same Cell
(one thread writes to Cell while the other thread reads from Cell).

#![forbid(unsafe_code)]
use buttplug::util::future::ButtplugFuture;
use futures::executor;

use std::cell::Cell;
use std::sync::Arc;
use std::thread;

#[derive(Debug, Clone, Copy)]
enum RefOrInt<'a> {
    Ref(&'a u64),
    Int(u64),
}
static X: u64 = 0;

fn main() {
    let future = ButtplugFuture::default();    
    let shared = future.get_state_clone();

    thread::spawn(move || {
        let shared = shared;

        let cell = Arc::new(Cell::new(RefOrInt::Int(0xdeadbeef)));
        shared.set_reply(Arc::clone(&cell));

        loop {
            cell.set(RefOrInt::Int(0xdeadbeef));
            cell.set(RefOrInt::Ref(&X))
        }
    });

    let smuggled_cell: Arc<Cell<RefOrInt>> = executor::block_on(future);
    println!("Future resolved");

    loop {
        if let RefOrInt::Ref(addr) = smuggled_cell.get() {
            if addr as *const _ as usize != 0xdeadbeef {
                continue;
            }
            // Due to the data race, obtaining Ref(0xdeadbeef) is possible
            println!("Pointer is now: {:p}", addr);
            println!("Dereferencing addr will now segfault: {}", *addr);
        }
    }
}

Suggested Solution

Adding T: Send bound to the Send impl & T:Sync bound to the Sync impl can
prevent code like the above to be revoked by the compiler.

unsafe impl<T: Send> Send for ButtplugFutureStateShared<T> {
}
unsafe impl<T: Sync> Sync for ButtplugFutureStateShared<T> {
}

Thank you for reviewing this issue 🦀

[JOE1994]

P.S. I used futures = "0.3.8" for building & running the above example program.

[qdot]

Thanks for the heads up! I'll check this out ASAP. :D

[Qwaz]

Is T: Sync for Sync bound correct? ButtplugFutureStateShared contains Arc<Mutex<ButtplugFutureState<T>>>, so I believe what the compiler infers is T: Send for Sync because of the trait bound of Mutex type.

If that's the case, these two unsafe lines can be removed entirely. The compiler will automatically apply the correct bound.

[JOE1994]

Is T: Sync for Sync bound correct? ButtplugFutureStateShared contains Arc<Mutex<ButtplugFutureState<T>>>, so I believe what the compiler infers is T: Send for Sync because of the trait bound of Mutex type.

If that's the case, these two unsafe lines can be removed entirely. The compiler will automatically apply the correct bound.

After removing the impl Send & impl Sync lines & locally building the documentation for this project,
I was able to confirm that you're right 👍

Below is a snapshot from the documentation I built locally after removing the two unsafe lines.