Bug 239550

Summary: std::thread::id uses pthread_equal in undefined ways not handled by _pthread_stubs
Product: Base System Reporter: andrew
Component: binAssignee: freebsd-threads (Nobody) <threads>
Status: Closed FIXED    
Severity: Affects Many People CC: dim, emaste, kib
Priority: --- Keywords: needs-patch, needs-qa
Version: CURRENTFlags: dim: mfc-stable12+
dim: mfc-stable11+
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239038
Attachments:
Description Flags
testcase (c++) none

Description andrew 2019-07-31 02:27:18 UTC
Created attachment 206172 [details]
testcase (c++)

std::thread::id defines a distinguished "not equal to any actual thread" value (the implementation uses 0).

But it feels entitled to pass this value to pthread_equal, whose own spec has no such distinguished value. (Per the pthread spec this gives an undefined result.)

If libthr is linked in, this isn't a problem because 0 doesn't actually compare equal to any thread. But the _pthread_stubs implementation of pthread_equal returns true unconditionally, which is valid by the pthread spec (since there is no distinguished pthread_t value).

The attached test program demonstrates the failure; it runs successfully if linked with -pthread, asserts out if not.

The simplest fix would be to do an actual comparison in the stub version of pthread_equal. A more correct fix would be for std::thread::id not to assume it can pass its 0 value to pthread_equal, but I think this would be unnecessary overhead.
Comment 1 Konstantin Belousov freebsd_committer 2019-07-31 19:46:57 UTC
There are some limits on how much efforts should we put on libc stubs when libthr is not linked in.  This configuration is only supported to make it easier to create e.g. loadable modules which must work correctly when dlopened either in threaded or in non-threaded process.

That said, if we start distinguishing the value 0, we get the next problem with pthread_getthreadid_np(3) which libc stub returns zero.

We might fix this eventually, if the anticipated merge of libthr into libc occurs. sometime.  Then it would work out of box.
Comment 2 andrew 2019-07-31 21:01:22 UTC
(In reply to Konstantin Belousov from comment #1)

pthread_getthreadid_np is irrelevant to this question since that is returning something completely different.

The referenced bug shows that this is a real live issue with ports, so it needs fixing, not just handwaving away with "link libthr into everything". There is no sane reason why a port like protobuf would have to link in libthr just to get std::thread::id() != std::this_thread::get_id()  to come out true.
Comment 3 Ed Maste freebsd_committer 2019-08-19 13:12:49 UTC
libcxx side of this issue has been fixed in https://github.com/llvm/llvm-project/commit/2e80d01fa7dc36c5f2533a363c6c0680f7896dc5#diff-a020123e9cf2d0b9d0d6f0ca236b6521 and will come in with a future import
Comment 4 commit-hook freebsd_committer 2019-08-20 17:40:01 UTC
A commit references this bug:

Author: dim
Date: Tue Aug 20 17:39:34 UTC 2019
New revision: 351253
URL: https://svnweb.freebsd.org/changeset/base/351253

Log:
  Pull in r368867 from upstream libc++ trunk (by Marshall Clow):

    Rework recursive_timed_mutex so that it uses __thread_id instead of
    using the lower-level __libcpp_thread_id. This is prep for fixing
    PR42918. Reviewed as https://reviews.llvm.org/D65895

  Pull in r368916 from upstream libc++ trunk (by Marshall Clow):

    Fix thread comparison by making sure we never pass our special 'not a
    thread' value to the underlying implementation. Fixes PR#42918.

  This should fix std::thread::id::operator==() attempting to call
  pthread_equal(3) with zero values.

  Reported by:	andrew@tao11.riddles.org.uk
  PR:		239038, 239550
  MFC after:	3 days

Changes:
  head/contrib/libc++/include/__threading_support
  head/contrib/libc++/include/mutex
  head/contrib/libc++/include/thread
  head/contrib/libc++/src/mutex.cpp
Comment 5 commit-hook freebsd_committer 2019-09-03 17:32:20 UTC
A commit references this bug:

Author: dim
Date: Tue Sep  3 17:31:14 UTC 2019
New revision: 351767
URL: https://svnweb.freebsd.org/changeset/base/351767

Log:
  MFC r351253:

  Pull in r368867 from upstream libc++ trunk (by Marshall Clow):

    Rework recursive_timed_mutex so that it uses __thread_id instead of
    using the lower-level __libcpp_thread_id. This is prep for fixing
    PR42918. Reviewed as https://reviews.llvm.org/D65895

  Pull in r368916 from upstream libc++ trunk (by Marshall Clow):

    Fix thread comparison by making sure we never pass our special 'not a
    thread' value to the underlying implementation. Fixes PR#42918.

  This should fix std::thread::id::operator==() attempting to call
  pthread_equal(3) with zero values.

  Reported by:	andrew@tao11.riddles.org.uk
  PR:		239038, 239550

Changes:
_U  stable/11/
  stable/11/contrib/libc++/include/__threading_support
  stable/11/contrib/libc++/include/mutex
  stable/11/contrib/libc++/include/thread
  stable/11/contrib/libc++/src/mutex.cpp
_U  stable/12/
  stable/12/contrib/libc++/include/__threading_support
  stable/12/contrib/libc++/include/mutex
  stable/12/contrib/libc++/include/thread
  stable/12/contrib/libc++/src/mutex.cpp