BiLockGuard allows data race due to insufficient bound

[Qwaz]

Bug Description

futures-rs/futures-util/src/lock/bilock.rs

Lines 48 to 49 in d74763c

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

BiLockGuard implements Send for T: Send and Sync for T: Send that are propagated from manual unsafe Send/Sync impl for Inner. However, BiLockGuard implements Deref for T which requires T: Sync bound for Sync. This definition allows parallel access to T: !Sync type, which is unsound.

Observation on impact

BiLock API is unstable, so hopefully the bug wouldn't affect a lot of people. I also lightly audited io split and stream split, two places where BiLock is internally used. All of them seem to call as_pin_mut() right after locking which I believe to be sound with the current definition (but it's not a guarantee!)

Proof of Concept

Tested with futures 0.3.21.

Code:

#![forbid(unsafe_code)]

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

use futures::lock::BiLock;

#[derive(Clone, Copy)]
enum RefOrInt {
    Ref(&'static u64),
    Int(u64),
}

static STATIC_INT: u64 = 123;

impl Default for RefOrInt {
    fn default() -> Self {
        RefOrInt::Ref(&STATIC_INT)
    }
}

#[tokio::main]
async fn main() {
    let (lock1, _lock2) = BiLock::new(Cell::new(RefOrInt::default()));

    // Contrived use case for easier demonstration
    let lock1 = Box::leak(Box::new(lock1));
    let guard1 = Arc::new(lock1.lock().await);
    let guard2 = guard1.clone();

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

    loop {
        if let RefOrInt::Ref(addr) = guard2.get() {
            if addr as *const u64 == &STATIC_INT as *const u64 {
                continue;
            }

            // We got Ref(0xdeadbeef) due to data race
            println!("Pointer is now: {:p}", addr);
            println!("Dereferencing addr will now segfault");
            println!("{}", addr);
        }
    }
}

Output:

Pointer is now: 0xdeadbeef
Dereferencing addr will now segfault
segmentation fault (core dumped)

[taiki-e]

@Qwaz Thanks for finding this! I filed #2570 to fix this.