Mapped*Guard objects should only be Send when the contained type is

[ammaraskar]

When using Sendable guard objects, such as with the send_guard feature or the locks in the spin crate, it is possible to get non-Sendable types across threads using the Mapped*Guard (MappedMutexGuard, MappedRwLockReadGuard, MappedRwLockWriteGuard) objects.

Currently, they're marked as Send as long as the raw lock is Send with no bound on T:

parking_lot/lock_api/src/mutex.rs

Lines 604 to 607 in 5ac2971

	unsafe impl<'a, R: RawMutex + 'a, T: ?Sized + 'a> Send for MappedMutexGuard<'a, R, T> where
	R::GuardMarker: Send
	{
	}

This should probably have a T: Send bound, just like the regular MutexGuard does. (Issue found by @sslab-gatech's Rust group)

---

Here's a proof-of-concept compiled with

parking_lot = { version = "=0.11.0", features = ['send_guard']}

using this issue to cause a data-race with a Cell object

#![forbid(unsafe_code)]
use std::cell::Cell;

use parking_lot::{Mutex, MutexGuard};
use crossbeam_utils::thread;

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

fn main() {
    let cell = Cell::new(RefOrInt::Ref(&SOME_INT));
    let mutex = Mutex::new(&cell);

    thread::scope(|s| {
        let guard = mutex.lock();
        // MappedMutexGuard that just returns the whole object as a "component".
        let mapped_guard = MutexGuard::map(guard, |x| x);

        let child = s.spawn(move |_| {
            let smuggled_cell = *mapped_guard;

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

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

This outputs:

Main - 0x7ffe6021e8a8 - Cell { value: Ref(123) }
Thread - 0x7ffe6021e8a8 - Cell { value: Ref(123) }
Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)

[Amanieu]

In the case of MappedRwLockReadGuard, should the requirement for Send be T: Send or T: Sync. I'm tending towards the latter since it only exposes &T.

[ammaraskar]

Hmm yeah for the read guard, since there can be multiple instances and we're exposing a &T, I think we should essentially follow Rust's bounds for Send on references: https://doc.rust-lang.org/std/marker/trait.Send.html#impl-Send-5

Which is, as you said, T: Sync