libstd Mutex can cause POSIX undefined behavior

[comex]

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.

[tomjakubowski]

From later in the document:

If an implementation detects that the value specified by the mutex argument to pthread_mutex_destroy() or pthread_mutex_init() refers to 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, or detects that the value specified by the mutex argument to pthread_mutex_init() refers to an already initialized mutex, it is recommended that the function should fail and report an [EBUSY] error.

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]

Glibc appears to check and return EBUSY for all but robust mutexes: pthread_mutex_destroy.c

Leaking in this case is trivial anyway, because it only wants to invalidate that mutex->__data.__kind.

[alexcrichton]

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]

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_destroy is literally return 0;: link.

[bluss]

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]

To quote MSDN "SRW locks do not need to be explicitly destroyed." and since we use SRW locks for Mutex everything is totally fine.

[bluss]

Dropping a mutex does call DeleteCriticalSection though, which is what I would look closer at.

[bluss]

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]

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]

And it's only XP, not much to worry about!

[cuviper]

Android's bionic implementation also looks harmless: pthread_mutex.cpp

[brson]

It sounds like there's nothing to be done here. Can we close this?