Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Our macOS os_unfair_lock do not complain when they are moved while being locked #3859

Closed
RalfJung opened this issue Sep 5, 2024 · 2 comments · Fixed by #3973
Closed

Our macOS os_unfair_lock do not complain when they are moved while being locked #3859

RalfJung opened this issue Sep 5, 2024 · 2 comments · Fixed by #3973
Labels
A-mac Area: Affects only macOS targets A-shims Area: This affects the external function shims C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 5, 2024

In #3745, the question came up what to do when a os_unfair_lock is moved. Apparently the real implementation uses the address of the lock to identify the wait queue, so a moved lock will be a "fork" of the old lock: it will start in whatever state the lock had at the time of the move, but reset the wait queue.

In Miri, we use a mutex ID tracked in the os_unfair_lock value to identify the lock, so the wait queue moves with the lock.

I don't think it makes sense to exactly replicate the macOS behavior here, but it would make sense to detect when a lock is moved, and throw an "unsupported" error. This could work similar to what #3784 does for pthread mutexes.

@RalfJung RalfJung added C-bug Category: This is a bug. A-shims Area: This affects the external function shims E-good-first-issue A good way to start contributing, mentoring is available A-mac Area: Affects only macOS targets labels Sep 5, 2024
@RalfJung
Copy link
Member Author

Behavior changed a bit with #3966: the queue no longer moves with the lock.

However, we should probably do something to detect a lock that is moved while being locked -- that seems like a rather dubious thing to do. The best way to do this probably is to store something non-zero in the lock whenever it is held, and to refuse initializing a new lock in os_unfair_lock_getid when the value is non-zero.

@RalfJung RalfJung changed the title Our macOS os_unfair_lock does not match platform behavior when the lock is moved Our macOS os_unfair_lock do not complain when they are moved while being locked Oct 14, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Oct 14, 2024

The best way to do this probably is to store something non-zero in the lock whenever it is held, and to refuse initializing a new lock in os_unfair_lock_getid when the value is non-zero.

Hm no that won't work -- one can leak the MutexGuard and then move the Mutex while it is "locked".

@joboet what is actually the behavior of that? Miri might be doing this wrong right now... calling lock() on such a moved-while-locked mutex should deadlock, but I think Miri will just give you the lock (since #3966). oops

use std::sync::Mutex;
use std::mem;

fn main() {
    let m = Mutex::new(0);
    mem::forget(m.lock());
    // Move the lock while it is "held" (really: leaked)
    let m2 = m;
    // Now try to acquire the lock again.
    let _guard = m2.lock();
}

@RalfJung RalfJung removed the E-good-first-issue A good way to start contributing, mentoring is available label Oct 14, 2024
@bors bors closed this as completed in 23ff2ea Oct 14, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this issue Oct 15, 2024
ensure that a macOS os_unfair_lock that is moved while being held is not implicitly unlocked

Fixes rust-lang/miri#3859

We mark an os_unfair_lock that is moved while being held as "poisoned", which means it is not considered forever locked. That's not quite what the real implementation does, but allowing arbitrary moves-while-locked would likely expose a ton of implementation details, so hopefully this is good enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mac Area: Affects only macOS targets A-shims Area: This affects the external function shims C-bug Category: This is a bug.
Projects
None yet
1 participant