Bug 234805 - pthread_*_destroy doesn't handle locked objects consistently
Summary: pthread_*_destroy doesn't handle locked objects consistently
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-threads mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-09 21:22 UTC by Mark Johnston
Modified: 2019-01-11 18:03 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Johnston freebsd_committer 2019-01-09 21:22:03 UTC
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.
Comment 1 Konstantin Belousov freebsd_committer 2019-01-10 04:18:08 UTC
(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.
Comment 2 Mark Johnston freebsd_committer 2019-01-11 15:53:38 UTC
(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.
Comment 3 Konstantin Belousov freebsd_committer 2019-01-11 17:46:13 UTC
(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.
Comment 4 Mark Johnston freebsd_committer 2019-01-11 18:03:25 UTC
(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.