MappedMutexGuard Send/Sync bound is unsound

[Qwaz]

Hello, we have noticed a soundness/memory safety issue in this crate which allows safe Rust code to trigger undefined behavior while scanning crates.io.

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

Lines 404 to 405 in 7340d3d

	unsafe impl<T: ?Sized + Send, U: ?Sized> Send for MappedMutexGuard<'_, T, U> {}
	unsafe impl<T: ?Sized + Sync, U: ?Sized> Sync for MappedMutexGuard<'_, T, U> {}

Description

Send/Sync implementation for MappedMutexGuard only considers variance on T, while MappedMutexGuard dereferences to U.
This can lead to data race in safe Rust code when a closure used in MutexGuard::map() returns U that is unrelated to T.

Demonstration

• Crate: futures
• Version: 0.3.6
• OS: Ubuntu 20.04.1 LTS
• Rust: rustc 1.47.0 (18bf6b4f0 2020-10-07)

#![forbid(unsafe_code)]

use crossbeam_utils::thread;
use futures::lock::{Mutex, MutexGuard};
use std::ops::Deref;
use std::rc::Rc;

const NUM_CLONES: usize = 1000000;

fn main() {
    let mutex = Mutex::new(true);
    let mutex_guard = futures::executor::block_on(mutex.lock());

    // T: bool, U: Rc<bool>
    // MappedMutexGuard is Send+Sync and Deref to U, due to:
    // unsafe impl<T: ?Sized + Send, U: ?Sized> Send for MappedMutexGuard<'_, T, U> {}
    // unsafe impl<T: ?Sized + Sync, U: ?Sized> Sync for MappedMutexGuard<'_, T, U> {}
    let mapped_mutex_guard = MutexGuard::map(mutex_guard, |_| Box::leak(Box::new(Rc::new(true))));

    // We demonstrate the issue by racing the non-atomic reference counter type `Rc` between two threads.
    thread::scope(|s| {
        let child = s.spawn(|_| {
            // &Rc<bool> sent to another thread
            let rc_ref = mapped_mutex_guard.deref();
            for _ in 0..NUM_CLONES {
                std::mem::forget(rc_ref.clone());
            }
        });

        // if `child.join().unwrap()` is here, the program succeeds

        let rc_ref = mapped_mutex_guard.deref();
        for _ in 0..NUM_CLONES {
            std::mem::forget(rc_ref.clone());
        }

        child.join().unwrap();

        // We made NUM_CLONES on both threads plus initial 1 reference.
        // But in reality we'll see that the strong_count varies across every run due to data race.
        assert_eq!(Rc::strong_count(rc_ref), 2 * NUM_CLONES + 1);
    })
    .unwrap();
}

Output:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1240979`,
 right: `2000001`', src/main.rs:64:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Return Code: 101

[taiki-e]

Good catch! Filed #2240 to fix this.