GenericMutexGuard should be marked !Sync

[ammaraskar]

I think the GenericMutexGuard object should me marked explicitly as !Sync, it looks like it was automatically given the Sync trait because it only consists of a GenericMutex reference which itself is Sync. However, the lock-guard object itself shouldn't be usable across threads because it assumes it has the lock acquired. (This issue was found by the Rust group at @sslab-gatech).

Here's a demonstration of how this can cause Cell, a non-Sync but Sendable type to be used across threads to create a data race:

#![forbid(unsafe_code)]

use futures_intrusive::sync::{GenericMutexGuard, Mutex};

use crossbeam_utils::thread;
use std::cell::Cell;

static SOME_INT: u64 = 123;

fn main() {
    #[derive(Debug, Clone, Copy)]
    enum RefOrInt<'a> {
        Ref(&'a u64),
        Int(u64),
    }
    let cell = Cell::new(RefOrInt::Ref(&SOME_INT));

    let futures_mutex: Mutex<Cell<_>> = Mutex::new(cell, false);
    let mutex_guard: GenericMutexGuard<_, Cell<_>> = futures_mutex.try_lock().unwrap();

    thread::scope(|s| {
        let guard_ref = &mutex_guard;
        let child = s.spawn(move |_| {
            let smuggled = &(**guard_ref);

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

        println!("In main: {:p} {:?}", &(*mutex_guard), *mutex_guard);
        loop {
            if let RefOrInt::Ref(addr) = mutex_guard.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);
            }
        }
    });
}

Output:

In main: 0x7ffc45082668 Cell { value: Ref(123) }
In the thread: 0x7ffc45082668 Cell { value: Ref(123) }
Pointer is now: 0xdeadbeef
Return Code: -11 (SIGSEGV)

[Matthias247]

You are right - this is wrong. The Send condition for the guard is correct, but the Sync one is broken.
Are you interested in creating a CR for fixing it?

[ammaraskar]

Hey @Matthias247, would you mind publishing a new version of this crate with this fix included?

[Matthias247]

0.4 is published: https://crates.io/crates/futures-intrusive