The POSIX entry for pthread_cond_destroy() says, "attempting to destroy a condition variable upon which other threads are currently blocked results in undefined behavior." Our man page does not say this and instead documents an error code of EBUSY as meaning "the variable cond is locked by another thread." However, the code does not attempt to detect this case and simply indicates success. OTOH, pthread_mutex_destroy() does check for this case and returns EBUSY. pthread_rwlock_destroy() has the same behaviour as pthread_cond_destroy(), and the compat10 sem_destroy() behaves like pthread_mutex_destroy(). We should be consistent about this.
(In reply to Mark Johnston from comment #0) For rwlock_destroy() this can be done, although I would be very careful with introducing such change. mutex_destroy() EINVAL caused too many grief. For cv_desstroy(), I do not see a good way to actually implement it. We have user/kernel sleeping threads, and although for user sleep this can be implemented with the same races as for mutex_destroy(), for kernel it seems to require new umtx op to cover just this case. I do not see it worth.
(In reply to Konstantin Belousov from comment #1) Sorry, can you explain why a new umtx op is needed? Can't the ucond's c_has_waiters field be used to determine whether threads are blocked in the kernel? Certainly this is racy if the thread calling pthread_cond_destroy() does not hold the associated mutex, but I can't see the need for a new umtx op.
(In reply to Mark Johnston from comment #2) My thought when I answered was that you do want to lock umtx queue lock before looking into the queue. If you are fine with the race described, so be it. The difference between mtx/rw and cv there is that most often case of destroying locked mutex and rwlock is when the thread that owns the lock destroys it, which makes the check relatively sane. For cv, thread which does destroy cannot sleep obviously so the check is always racy.
(In reply to Konstantin Belousov from comment #3) Well, today, if an unlocked pthread_cond_destroy() races with pthread_cond_wait(), there is a possibility of a use-after-free. The proposal does not introduce any new race AFAIK, though I did not yet look closely at the pshared case.
I have an older patch to add an EBUSY check for pthread_cond. I just never got around to writing a test case for it and testing it. I'll see if I can't dig the patch set up.
(In reply to John Baldwin from comment #5) I wrote this: https://github.com/markjdb/freebsd-dev/commit/f9e7418a0fa20f312ca106ea5c5839fb2f95a986 The test is unrelated and doesn't exercise the libthr diff, another test needs to be written.
A commit references this bug: Author: markj Date: Fri Mar 8 21:07:09 UTC 2019 New revision: 344935 URL: https://svnweb.freebsd.org/changeset/base/344935 Log: Have pthread_cond_destroy() return EBUSY if the condvar has waiters. This is not required of a compliant implementation, but it's easy to check for and helps improve compatibility with other common implementations. Moreover, it's consistent with our pthread_mutex_destroy(). PR: 234805 Reviewed by: jhb, kib, ngie MFC after: 2 weeks Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D19496 Changes: head/contrib/netbsd-tests/lib/libpthread/t_cond.c head/lib/libthr/thread/thr_cond.c
A commit references this bug: Author: markj Date: Mon Mar 25 14:05:36 UTC 2019 New revision: 345501 URL: https://svnweb.freebsd.org/changeset/base/345501 Log: MFC r344935: Have pthread_cond_destroy() return EBUSY if the condvar has waiters. PR: 234805 Changes: _U stable/12/ stable/12/contrib/netbsd-tests/lib/libpthread/t_cond.c stable/12/lib/libthr/thread/thr_cond.c
Do the commits in comment #7 and comment #8 resolve this issue? Can we close it?
I think it can't be closed yet as pthread_rwlock_destroy() is now the only one without an EBUSY check?
^Triage: is this aging PR still relevant?