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-03-25 14:06 UTC (History)
2 users (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.
Comment 5 John Baldwin freebsd_committer freebsd_triage 2019-02-01 17:12:09 UTC
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.
Comment 6 Mark Johnston freebsd_committer 2019-02-01 18:25:14 UTC
(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.
Comment 7 commit-hook freebsd_committer 2019-03-08 21:07:37 UTC
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
Comment 8 commit-hook freebsd_committer 2019-03-25 14:06:35 UTC
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