Allow owned data in MappedMutexGuard #290

eira-fransham · 2021-06-21T12:36:07Z

Closes #289

bjorn3 · 2021-06-21T12:48:50Z

lock_api/src/mutex.rs

    {
        let raw = &s.mutex.raw;
-        let data = f(unsafe { &mut *s.mutex.data.get() });
+        let data = unsafe { &mut *s.mutex.data.get() };
        mem::forget(s);


This will invalidate the data reference.

I don't think that is true, forget doesn't invalidate anything. For example, Box::leak leaks the value and returns a borrow of any length.

You are right. This doesn't actually invalidate the borrow because s.mutex is a reference.

bjorn3 · 2021-06-21T12:49:47Z

lock_api/src/mutex.rs

-        let data = match f(unsafe { &mut *s.mutex.data.get() }) {
+        let data = unsafe { &mut *s.mutex.data.get() };
+
+        let data = match f(data) {


This will leak the mutex guard if f(data) panics.

I'm trying to work out what the correct behaviour here would actually be - I've tried to work out a way to cause an invalid reference using the f closure but as far as I see it the lifetime bounds make it fine. I think just using mem::forget(s) after the match instead of using mem::ManuallyDrop is the correct behaviour.

I would have preferred to use catch_unwind/resume_unwind to make the control flow more explicit but unfortunately that's not available with no_std right now.

bjorn3 · 2021-06-21T12:50:35Z

lock_api/src/mutex.rs

+
+        let data = unsafe { ptr::read(&mut s.data) };
+
+        let data = f(data);


There is a potential double-drop here: if f panics then data will be dropped again despite having already been read from s.data.

bjorn3 · 2021-06-21T12:50:51Z

lock_api/src/mutex.rs

-        })
+        let data = unsafe { ptr::read(&mut s.data) };
+
+        f(data)


eira-fransham · 2021-06-23T11:18:55Z

@bjorn3 Thank you for your review. I've also noticed that I accidentally based this PR on my other PR implementing the bytemuck traits instead of on master, so I'll fix that too. Would you say that you're in favour of the general idea of the PR, even if there are issues with the implementation? I believe that it's a minor breaking change since it will affect anyone explicitly specifying the type and lifetime parameters of MappedMutexGuard::map/try_map.

bjorn3 · 2021-06-23T11:21:38Z

I think it would be fine, but don't know enough about parking-lot's internals to judge.

Amanieu · 2021-06-23T12:55:42Z

I'm very much in favor of this change. In fact I would like to make this a breaking change and directly change MappedMutexGuard instead of adding OwnedMappedMutexGuard.

Could you also extend this to support other lock types (rwlock, reentrant mutex, etc)?

yume-chan · 2021-08-09T07:43:52Z

I also want this. My mapper f returns a Box<T> so I need the guard to own the data. However, instead of Mutex I'm using RwLock.

Amanieu · 2021-08-09T13:04:47Z

@Vurich Are you still working on this PR?

eira-fransham · 2021-08-09T13:08:12Z

I’m sorry, I was working on this as part of a project at work but we went with a solution that no longer needed this change anyway, so I forgot about it. I can finish up this PR and we can get it merged.

Amanieu · 2021-08-09T18:02:48Z

Thanks, that would be great!

Amanieu · 2021-08-23T16:05:52Z

Hi, are you still updating this PR? Otherwise I can take over, finish it and get it merged.

Amanieu · 2021-08-25T00:23:11Z

lock_api/src/mutex.rs

    where
-        F: FnOnce(&mut T) -> &mut U,
+        F: FnOnce(&'a mut T) -> U,


This is almost certainly wrong: 'a is the lifetime of the Mutex object, which can be 'static.

Suggested change

F: FnOnce(&'a mut T) -> U,

F: FnOnce(&mut T) -> U,

'a is the lifetime of the MutexGuard's lock of the Mutex, which isn't the same and to my knowledge it is valid to get an &'a mut T from a MutexGuard<'a, _, T> since for the duration of 'a the only reachable path to the T is via the MutexGuard, and the MutexGuard is consumed in this method. Can you come up with an example of a piece of invalid code that would be enabled by this?

So I gave it some thought and I think I'll remove the lifetime, it's the more-conservative and more-obviously-correct option and it still allows for 99% of valid usecases, most importantly MutexGuard::map(foo, |a| &mut a.some_field). We can generalise f to be a 'a-taking function later if it proves to be a pain point, but it's not worth the possibility of causing UB.

#[test] fn test_map() { let mut outer = &mut 0; static M: Mutex<i32> = Mutex::const_new(super::RawMutex::INIT, 0); let guard = super::MutexGuard::map(M.lock(), |inner: &'static mut i32| { outer = inner; }); drop(guard); *outer = 1; }

'a is the lifetime of the Mutex. The lifetime of the lock is the lifetime of self.

This approach appears to work and disallows the code we want to disallow while allowing the code we want to allow, but the ergonomics are quite bad (I'll explain what the problem is below):

pub trait FnOnceShim<'a, T: 'a> { type Output: 'a; fn call(self, input: T) -> Self::Output; } impl<'a, F, In, Out> FnOnceShim<'a, In> for F where F: FnOnce(In) -> Out, In: 'a, Out: 'a, { type Output = Out; fn call(self, input: In) -> Self::Output { self(input) } } impl<'a, R: RawMutex + 'a, T: ?Sized + 'a> MutexGuard<'a, R, T> { // ... pub fn map<F>( s: Self, f: F, ) -> MappedMutexGuard<'a, R, <F as FnOnceShim<'a, &'a mut T>>::Output> where for<'any> F: FnOnceShim<'any, &'any mut T>, { // ... } // ... } impl<'a, R: RawMutex + 'a, T: 'a> MappedMutexGuard<'a, R, T> { // ... pub fn map<F>(s: Self, f: F) -> MappedMutexGuard<'a, R, <F as FnOnceShim<'a, T>>::Output> where for<'any> F: FnOnceShim<'any, T>, { // ... } // ... }

The issue is that Rust will no longer automatically convert the closure to the most-general version of it like it will when being passed as an F: FnOnce(..) -> ... Therefore, you need to do something like this:

MutexGuard::map(M.lock(), { |inner| inner } as fn(&mut usize) -> &mut usize)

Would it help if you changed the bounds to contain both FnOnce and FnOnceShim? Even though it is redundant, it may help with type inference.

Unfortunately that doesn't work - you would have to bound for<'any> F: FnOnce<(&'any mut T,)> because using the stable F: FnOnce(..) -> .. syntax you need to specify the return type, which means the caller must specify a concrete lifetime. The only bound that would work is:

pub fn map<U, F>( s: Self, f: F, ) -> MappedMutexGuard<'a, R, <F as FnOnceShim<'a, &'a mut T>>::Output> where for<'any> F: FnOnceShim<'any, &'any mut T>, F: FnOnce(&'a mut T) -> U, { // .. }

But then we're back to square one since that will make Rust infer the smaller bound, which is the problem we were having in the first place. I'll see if the shim fn traits in the fn_ops crate fixes the issue.

Note: you can also work around this with impl Trait, but it's only slightly less clunky

let m = Mutex::new((0, 0)); fn make_mapper() -> impl for<'any> FnOnce(&'any mut (usize, usize)) -> &'any mut usize { |inner: &mut (_, _)| &mut inner.0 } let mapper = make_mapper(); let guard = MutexGuard::map(m.lock(), mapper);

And you can also use a local function with manually-specified trait bounds, but client code needs to write this manually since if you're returning e.g. a Ref<'_, T> then you will need to specify that (discovered in rust-lang/rust#58052)

fn annotate<T, U, F>(f: F) -> F where F: FnOnce(&mut T) -> &mut U, { f } let m = Mutex::new((0, 0)); let mapper = annotate(|inner: &mut (_, _)| &mut inner.0); let guard = MutexGuard::map(m.lock(), mapper);

Ok, so I hit a new problem - in order to support try_map, we essentially either need a new trait specialised to try_map or we need to use the try_trait_v2 and control_flow_enum features. Here's what those options look like:

New trait

pub trait FnOnceOptionShim<'a, T: 'a> { type Output: 'a; fn call(self, input: T) -> Option<Self::Output>; } impl<'a, F, In, Out> FnOnceOptionShim<'a, In> for F where F: FnOnce(In) -> Option<Out>, In: 'a, Out: 'a, { type Output = Out; fn call(self, input: In) -> Option<Self::Output> { self(input) } } pub fn try_map<F>( s: Self, f: F, ) -> Result< MappedRwLockReadGuard<'a, R, <F as FnOnceOptionShim<'a, &'a T>>::Output>, Self, > where for<'any> F: FnOnceOptionShim<'any, &'any T>, { let raw = &s.rwlock.raw; let data = match f.call(unsafe { &*s.rwlock.data.get() }) { Some(data) => data, None => return Err(s), }; mem::forget(s); Ok(MappedRwLockReadGuard { raw, data, marker: PhantomData, }) }

try_trait_v2 + control_flow_enum

pub fn try_map<F>( s: Self, f: F, ) -> Result< MappedRwLockReadGuard< 'a, R, <<F as FnOnceShim<'a, &'a T>>::Output as core::ops::Try>::Output, >, Self, > where for<'any> F: FnOnceShim<'any, &'any T>, for<'any> <F as FnOnceShim<'any, &'any T>>::Output: core::ops::Try, { use core::ops::{ControlFlow, Try}; let raw = &s.rwlock.raw; let data = match Try::branch(FnOnceShim::call(f, unsafe { &*s.rwlock.data.get() })) { ControlFlow::Continue(data) => data, ControlFlow::Break(_) => return Err(s), }; mem::forget(s); Ok(MappedRwLockReadGuard { raw, data, marker: PhantomData, }) }

Amanieu · 2021-08-25T00:24:10Z

lock_api/src/mutex.rs

    where
-        F: FnOnce(&mut T) -> Option<&mut U>,
+        F: FnOnce(&'a mut T) -> Option<U>,


Amanieu · 2021-08-25T00:27:37Z

lock_api/src/mutex.rs

-    data: *mut T,
-    marker: PhantomData<&'a mut T>,
+    // We use `&'a mut` to make this type invariant over `'a`
+    marker: PhantomData<&'a mut ()>,


Does it actually need to be invariant here? I think we can safely remove this.

I kept it invariant because that was how it was in the original code, and it's the most conservative option. Without it this type would be variant, which is probably correct but there's a chance of it being subtly broken.

Actually this made T invariant, not 'a (see https://doc.rust-lang.org/nomicon/subtyping.html#variance).

The proper way to get invariance on 'a is PhantomData<fn(&'a ()) -> &'a ()>

Amanieu · 2021-08-25T00:32:19Z

lock_api/src/mutex.rs

@@ -780,10 +788,12 @@ impl<R: RawMutex, T: ?Sized> Drop for ArcMutexGuard<R, T> {
 /// could introduce soundness issues if the locked object is modified by another
 /// thread.
 #[must_use = "if unused the Mutex will immediately unlock"]
-pub struct MappedMutexGuard<'a, R: RawMutex, T: ?Sized> {
+pub struct MappedMutexGuard<'a, R: RawMutex, T: ?Sized + 'a> {


I don't think T has to live as long as 'a, we can remove the 'a bound on T.

Only R needs the bound because we have a &'a R.

It definitely doesn't need the 'a bound on the struct itself, and I think you're right that it isn't needed elsewhere either.

Amanieu · 2021-08-25T00:33:59Z

lock_api/src/mutex.rs

    {
+        use core::ptr;


Move this to the top of the file.

Amanieu · 2021-08-25T00:35:36Z

lock_api/src/mutex.rs

+
+        let data = unsafe { ptr::read(&mut s.data) };
+
+        let data = f(data);


There is a potential double-drop here: if f panics then data will be dropped again despite having already been read from s.data.

Amanieu · 2021-08-25T00:35:42Z

lock_api/src/mutex.rs

        let raw = s.raw;
-        let data = f(unsafe { &mut *s.data });
+
+        let data = unsafe { ptr::read(&mut s.data) };


Suggested change

let data = unsafe { ptr::read(&mut s.data) };

let data = unsafe { ptr::read(&s.data) };

…been proven safe

Amanieu · 2021-08-30T23:14:31Z

Unfortunately the need to cast the closure to a fn is a dealbreaker: it makes the interface unergonomic and doesn't work with closures that capture state.

eira-fransham · 2021-08-31T08:59:08Z

Unfortunately the need to cast the closure to a fn is a dealbreaker: it makes the interface unergonomic and doesn't work with closures that capture state.

You don't need to cast to fn, you just need to force Rust to infer the most-general lifetime for the argument. There are a couple of ways to do this, but they all work in essentially the same way:

// Option 1
fn annotate<F, T, U>(f: F) -> F
where
    F: FnOnce(&mut T) -> &mut U,
{
    f
}

let func = annotate(|foo: &mut (_, _)| &mut foo.0);

// Option 2
fn create_mapping_func<A, B>() -> impl FnOnce(&mut (A, B)) -> A {
    |foo| &mut foo.0
}

let func = create_mapping_func();

// Option 3
let func: &mut dyn FnMut(&mut (_, _)) -> &mut _ = &mut |foo| &mut foo.0;

// Option 4
let func: fn(&mut (_, _)) -> &mut _ = (|foo| &mut foo.0) as _;

All of these are pretty bad though, sadly. Rust has a long-standing issue open to fix this, but it has yet to be addressed rust-lang/rust#70263

If it was only required to do this for simple cases, then there would be no problem. The issue is that it's required for all cases. Maybe we could add map_mut and map_ref methods which specialise for FnOnce(&mut T) -> &mut U and FnOnce(&T) -> &U respectively? Again, not great, but it would mean that simple cases still function correctly. You would probably need to add the same for Mapped*Guard, too.

bjorn3 reviewed Jun 21, 2021

View reviewed changes

lock_api/src/mutex.rs Outdated

})

let data = unsafe { ptr::read(&mut s.data) };

f(data)

Copy link

Contributor

bjorn3 Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

eira-fransham force-pushed the allow-owned-mappedmutexguard branch from 4822084 to 5dda2cd Compare August 24, 2021 11:37

eira-fransham mentioned this pull request Aug 24, 2021

CI is running an extremely old version of Rust #295

Closed

eira-fransham force-pushed the allow-owned-mappedmutexguard branch from 5dda2cd to ee82b92 Compare August 24, 2021 12:30

Amanieu reviewed Aug 25, 2021

View reviewed changes

eira-fransham added 4 commits August 25, 2021 11:05

Allow MappedMutexGuard to own its data

4223cd6

Change MappedRwLock types to allow owned data

8a598ca

Fix double drop in Mapped*Guard::{try_,}map

a89fb5c

Restrict mapping function bounds to prevent possible UB until it has …

de6920c

…been proven safe

eira-fransham force-pushed the allow-owned-mappedmutexguard branch from 825f800 to de6920c Compare August 25, 2021 09:05

WIP: Add more lifetime bounds

e2fa040

eira-fransham mentioned this pull request Aug 26, 2021

HRTBs: "implementation is not general enough", but it is rust-lang/rust#70263

Open

fix: Fix lifetime issues (at the cost of worse type inference)

9f596ca

Remove unnecessary fn_ops dependency

7f6f778

xmo-odoo mentioned this pull request Nov 26, 2021

Consider allowing owned data in MappedMutexGuard #289

Open

Amanieu mentioned this pull request Oct 17, 2023

Allow indirect/partially owned mapping #408

Open


		let data = unsafe { ptr::read(&mut s.data) };

		let data = f(data);

	let data = unsafe { ptr::read(&mut s.data) };
	let data = unsafe { ptr::read(&s.data) };

Allow owned data in MappedMutexGuard #290

Are you sure you want to change the base?

Allow owned data in MappedMutexGuard #290

Conversation

eira-fransham commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham commented Jun 23, 2021

bjorn3 commented Jun 23, 2021

Amanieu commented Jun 23, 2021

yume-chan commented Aug 9, 2021

Amanieu commented Aug 9, 2021

eira-fransham commented Aug 9, 2021

Amanieu commented Aug 9, 2021

Amanieu commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Aug 26, 2021 • edited

Choose a reason for hiding this comment

eira-fransham Aug 26, 2021 • edited

Choose a reason for hiding this comment

eira-fransham Aug 26, 2021 • edited

Choose a reason for hiding this comment

New trait

try_trait_v2 + control_flow_enum

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Aug 30, 2021

eira-fransham commented Aug 31, 2021 • edited

eira-fransham Aug 25, 2021 •

edited

eira-fransham Aug 25, 2021 •

edited

eira-fransham Aug 26, 2021 •

edited

eira-fransham Aug 26, 2021 •

edited

eira-fransham Aug 26, 2021 •

edited

`try_trait_v2` + `control_flow_enum`

eira-fransham commented Aug 31, 2021 •

edited