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.