Repository navigation
Destroying locked Mutex in libstd triggers miri in safe code #85434
Description
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).
Activity
hameerabbasi commentedon May 18, 2021
Assigning a priority according to the WG-prioritization discussion on Zulip and removing
I-prioritize.@rustbot modify labels +P-high -I-prioritize
RalfJung commentedon May 22, 2021
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_destroywhen the lock is still held? Seems hard to do that atomically...chorman0773 commentedon May 22, 2021
nbdd0121 commentedon May 26, 2021
We could
pthread_mutex_trylock, and if we successfully acquire the lock, dopthread_mutex_unlockandpthread_mutex_destroy. If we couldn't acquire the lock, just leak...RalfJung commentedon May 26, 2021
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 Mutexhere.So, yeah, that could work.
m-ou-se commentedon May 27, 2021
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 commentedon May 27, 2021
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 commentedon May 27, 2021
Oh, right. I misread the comment. :)
Then the only question that remains is whether it's okay to not call
pthread_mutex_destroybut 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 commentedon May 27, 2021
It probably has to be properly leaked. I would assume that the raw mutex needs to be properly treated as
Pinned.4 remaining items
m-ou-se commentedon May 27, 2021
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 commentedon May 31, 2021
Lowering priority to P-medium because of the previous discussion in #31936.
DemiMarie commentedon Jan 28, 2022
I honestly think this is the only reasonable solution. POSIX’s semantics are not a good fit for us.
m-ou-se commentedon Jan 28, 2022
We discussed this in a recent library team meeting, and we'll be going forward with this.
DemiMarie commentedon Jan 28, 2022
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 commentedon Feb 7, 2022
That effort is now tracked here: #93740
m-ou-se commentedon Apr 13, 2022
This has been fixed on Linux. We no longer use pthread locks on Linux. :)
Rollup merge of rust-lang#98194 - m-ou-se:leak-locked-pthread-mutex, …
Rollup merge of rust-lang#98194 - m-ou-se:leak-locked-pthread-mutex, …
Rollup merge of rust-lang#98194 - m-ou-se:leak-locked-pthread-mutex, …
Rollup merge of #98194 - m-ou-se:leak-locked-pthread-mutex, r=Amanieu