Destroying locked Mutex in libstd triggers miri in safe code

[chorman0773]

I tried this code through miri:

use std::sync::Mutex;

fn main(){
    let m = Mutex::new(5i32);
    
    core::mem::forget(m.lock());
}

I expected to see this happen: No observable behaviour, including from miri (aside from "Unsupported Operation" errors).

Instead, this happened:
miri reports undefined behaviour in "Destroying locked mutex" when calling pthread_mutex_destroy (Note: this report is correct, calling pthread_mutex_destroy on a locked mutex is prescribed to be undefined behaviour by POSIX)

Meta

This was tested on all latest versions of rustc, all using miri 0.1.54, on play.rust-lang.org:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=28904dec86ec2f64bb03163bedf37299

Miri Backtrace

error: Undefined Behavior: destroyed a locked mutex
  --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/mutex.rs:78:17
   |
78 |         let r = libc::pthread_mutex_destroy(self.inner.get());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ destroyed a locked mutex
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
           
   = note: inside `std::sys::unix::mutex::Mutex::destroy` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/mutex.rs:78:17
   = note: inside `<std::sys_common::mutex::MovableMutex as std::ops::Drop>::drop` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/mutex.rs:98:18
   = note: inside `std::ptr::drop_in_place::<std::sys_common::mutex::MovableMutex> - shim(Some(std::sys_common::mutex::MovableMutex))` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:192:1
   = note: inside `std::ptr::drop_in_place::<std::sync::Mutex<i32>> - shim(Some(std::sync::Mutex<i32>))` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:192:1
note: inside `main` at src/main.rs:8:1
...
[9 frames before `main` omitted manually]

This is caused by issue presented in #31936. However, I believe that it deserves new attention given that it causes miri to fail in safe code (and is not a miri false positive, as miri is correctly reporting undefined behaviour in calling pthread_mutex_destroy).

[hameerabbasi]

Assigning a priority according to the WG-prioritization discussion on Zulip and removing I-prioritize.

@rustbot modify labels +P-high -I-prioritize

[RalfJung]

However, I believe that it deserves new attention given that it causes miri to fail in safe code

Well, Miri is just a tool to detect issues like this. It looks like back then the decision was made to ignore what POSIX says and instead to rely on what certain implementations do. Since Miri checks POSIX compliance as strictly as it can, this leads to Miri errors in safe code.

Ideally we'd work towards getting the spec changed (there seems to be no good reason for this UB, and without something like Rust it is unclear why anyone in reasonable code would even want to do this). That's probably very hard though. Are there work-arounds where we can avoid calling pthread_mutex_destroy when the lock is still held? Seems hard to do that atomically...

[chorman0773]

On Sat, May 22, 2021 at 04:23 Ralf Jung ***@***.***> wrote: However, I believe that it deserves new attention given that it causes miri to fail in safe code Well, Miri is just a tool to detect issues like this. It looks like back then the decision was made to ignore what POSIX says and instead to rely on what certain implementations do. Since Miri checks POSIX compliance as strictly as it can, this leads to Miri errors in safe code.

Indeed. And, as I've mentioned in the thread, I wouldn't consider this a bug in miri, for precisely that reason.

Ideally we'd work towards getting the spec changed (there seems to be no good reason for this UB, and without something like Rust it is unclear why anyone in reasonable code would even want to do this).

One half of it could be destroying it *potentially concurrent* to it becoming locked, since destruction is presumably not atomic, though this wouldn't be an issue for rust. Another reasonable possibility would be for enhanced implementations that, for example, check and trap deadlocks, but as a consequence, couldn't permit being destroyed while locked (the issue was discussed somewhat on the Rust Community Discord, in the #black-magic channel, and this was one potential reason for the POSIX UB).

That's probably very hard though. Are there work-arounds where we can avoid calling pthread_mutex_destroy when the lock is still held? Seems hard to do that atomically...

I've actually considered this previously for my implementation, and that's really the only option to avoid strict UB, without fixing the spec (which will take some time, though I don't know how much). Since the destructor is &mut self, it doesn't need to be atomic. It could just be an AtomicBool (or any other atomic, possibly AtomicPtr, which IIRC is the only atomic dependency in libstd) that gets set on being locked, and cleared on unlock.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#85434 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD22QU5RRFLFLYZN4VELTO5SYDANCNFSM45CJNX2A> .

[nbdd0121]

We could pthread_mutex_trylock, and if we successfully acquire the lock, do pthread_mutex_unlock and pthread_mutex_destroy. If we couldn't acquire the lock, just leak...

[RalfJung]

I was about to ask what if someone else acquires the lock again just after we pthread_mutex_unlock... but that cannot happen since we have &mut Mutex here.

So, yeah, that could work.

[m-ou-se]

do pthread_mutex_unlock and

Posix says it's UB to call that function on a different thread than the one that locked the mutex, so that wouldn't work, unfortunately.

[chorman0773]

I assume that was in the case that successfully called pthread_mutex_trylock, so it's being unlocked on the thread that locked it.

[m-ou-se]

Oh, right. I misread the comment. :)

Then the only question that remains is whether it's okay to not call pthread_mutex_destroy but still drop/reuse the memory. Without calling destroy, pthread doesn't know that memory is no longer a mutex it is managing. If it has some kind of linked list of mutexes or something, that could result in UB.

[chorman0773]

It probably has to be properly leaked. I would assume that the raw mutex needs to be properly treated as Pinned.

[nbdd0121]

We can leak the box or drop the box depending on whether it's UB or not.

[nbdd0121]

Attempting to initialize an already initialized mutex results in undefined behavior.

We have to leak the box.

[m-ou-se]

A more extreme option is to get rid of pthread mutexes entirely. (So, implement our mutex/rwlock/condvars directly using atomics and platform-specific syscalls, like we already do for thread::park() on most platforms.) That's still somewhere on my to do list. ^^'

[RalfJung]

You mean like #56410 (not sure if there's an issue for that)?

[m-ou-se]

Yeah, either parking_lot, or something other thread parker based, or maybe something not thread parker based but directly using a futex (or whatever the platform supports) might be a better fit for std. It's a bit tricky to make those trade offs for all std users at once, but not having to deal with all this pthread stuff (and not having to Box them, etc.) would be very nice.

[m-ou-se]

Lowering priority to P-medium because of the previous discussion in #31936.

[DemiMarie]

A more extreme option is to get rid of pthread mutexes entirely. (So, implement our mutex/rwlock/condvars directly using atomics and platform-specific syscalls, like we already do for thread::park() on most platforms.) That's still somewhere on my to do list.

I honestly think this is the only reasonable solution. POSIX’s semantics are not a good fit for us.

[m-ou-se]

A more extreme option is to get rid of pthread mutexes entirely. (So, implement our mutex/rwlock/condvars directly using atomics and platform-specific syscalls, like we already do for thread::park() on most platforms.)

[..] either parking_lot, or something other thread parker based, or maybe something not thread parker based but directly using a futex (or whatever the platform supports) [..]

We discussed this in a recent library team meeting, and we'll be going forward with this.

[DemiMarie]

A more extreme option is to get rid of pthread mutexes entirely. (So, implement our mutex/rwlock/condvars directly using atomics and platform-specific syscalls, like we already do for thread::park() on most platforms.)

We discussed this in a recent library team meeting, and we'll be going forward with this.

Yay!!! That gives Rust an opportunity to have better performance than pthreads, too. The one caveat is robust mutexes, but I don’t think Rust needs those.

[m-ou-se]

A more extreme option is to get rid of pthread mutexes entirely. (So, implement our mutex/rwlock/condvars directly using atomics and platform-specific syscalls, like we already do for thread::park() on most platforms.)

[..] either parking_lot, or something other thread parker based, or maybe something not thread parker based but directly using a futex (or whatever the platform supports) [..]

We discussed this in a recent library team meeting, and we'll be going forward with this.

That effort is now tracked here: #93740

[m-ou-se]

This has been fixed on Linux. We no longer use pthread locks on Linux. :)