Repository navigation
libstd Mutex can cause POSIX undefined behavior #31936
Description
Not sure if anything needs to be done about this, since I don't know of any actual implementations having a problem with it, but it occurred to me when thinking about moving mutexes.
On Unix, Mutex::new corresponds to initialization with PTHREAD_MUTEX_INITIALIZER; Mutex::lock, which returns a MutexGuard, corresponds to pthread_mutex_lock; MutexGuard::drop corresponds to pthread_mutex_unlock; and Mutex::drop corresponds to pthread_mutex_destroy.
If a MutexGuard is forgotten:
use std::sync::Mutex;
use std::mem::forget;
fn main() {
let mutex: Mutex<()> = Mutex::new(());
let guard = mutex.lock();
forget(guard);
}...then we call pthread_mutex_destroy without first calling pthread_mutex_unlock. The POSIX spec says:
It shall be safe to destroy an initialized mutex that is unlocked. Attempting to destroy a locked mutex or a mutex that is referenced (for example, while being used in a pthread_cond_timedwait() or pthread_cond_wait()) by another thread results in undefined behavior.
Activity
tomjakubowski commentedon Feb 28, 2016
From later in the document:
That's interesting, do Rust's supported platforms follow that recommendation? I guess in that case leaking the guard would end up leaking the mutex as well.
cuviper commentedon Feb 28, 2016
Glibc appears to check and return
EBUSYfor all but robust mutexes: pthread_mutex_destroy.cLeaking in this case is trivial anyway, because it only wants to invalidate that
mutex->__data.__kind.alexcrichton commentedon Feb 28, 2016
Bah oh well. I suspect that the undefined behavior here is written in with the intention of not wanting to specify what it means to unlock a destroyed mutex. As in the specific failure mode for us is that we destroy a locked mutex but still guarantee that it will never be used again. I suspect this is probably not a problem in practice, but would be good to handle nonetheless! We can likely just get away with a flag that indicates whether the lock has any active holders or not and we just avoid calling the destroy function if so.
If @tomjakubowski is right though we can probably just close this or maybe fix a debug assertion in the destructor.
comex commentedon Feb 28, 2016
OS X is also in the "returns EBUSY if locked, doesn't actually need to free resources anyway" camp: see pthread_mutex_destroy.
musl's
pthread_mutex_destroyis literallyreturn 0;: link.bluss commentedon Feb 28, 2016
The Windows api also has a comment about destroying a locked mutex, but it seems to refer only to undefined-ness of other active users of the same mutex. (Requires a Windows-savvy developer to do the real interpretation though).
retep998 commentedon Feb 28, 2016
To quote MSDN "SRW locks do not need to be explicitly destroyed." and since we use SRW locks for
Mutexeverything is totally fine.bluss commentedon Feb 28, 2016
Dropping a mutex does call
DeleteCriticalSectionthough, which is what I would look closer at.bluss commentedon Feb 28, 2016
It just says "If a critical section is deleted while it is still owned, the state of the threads waiting for ownership of the deleted critical section is undefined."
retep998 commentedon Feb 28, 2016
Oh, we do use critical sections for the fallback implementation, but that only occurs on Windows XP and older. Also when we destroy a mutex we know nobody is currently waiting for ownership, as clearly we can only destroy it when there are no other owners. If another thread has leaked the guard, it isn't waiting for ownership so that sentence doesn't apply to it.
bluss commentedon Feb 28, 2016
And it's only XP, not much to worry about!
cuviper commentedon Feb 28, 2016
Android's bionic implementation also looks harmless: pthread_mutex.cpp
brson commentedon Mar 2, 2016
It sounds like there's nothing to be done here. Can we close this?