[Bug libstdc++/96817] New: __cxa_guard_acquire unsafe against dynamically loaded pthread

[Bug libstdc++/96817] New: __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

            Bug ID: 96817
           Summary: __cxa_guard_acquire unsafe against dynamically loaded
                    pthread
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: yshuiv7 at gmail dot com
  Target Milestone: ---

If pthread is loaded dynamically via dlopen() after libstdc++ is loaded,
__gthread_active_p() would return wrong result, cause guard acquire to be
thread unsafe.

An example of this problem:
https://gitlab.freedesktop.org/mesa/mesa/-/issues/3199

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #1 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
Example:

This program normally deadlocks when using linked pthread:

https://godbolt.org/z/Yrza4e

But it will throw recursive_init_error when using dlopen'd pthread:

https://godbolt.org/z/afMe5d

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Dynamically loading libpthread breaks everything in libstdc++, it just isn't
supported.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #3 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Jonathan Wakely from comment #2)
> Dynamically loading libpthread breaks everything in libstdc++, it just isn't
> supported.

But it has real use cases, e.g. writing a C++ OpenGL program that doesn't use
thread itself.

This is such an easy hole for the end user to fall into. How would they know
that they have to link against pthread even though their program doesn't use
any threading?

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
it's been always that way - but yes, people repeatedly fall into this trap

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #5 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Richard Biener from comment #4)
> it's been always that way - but yes, people repeatedly fall into this trap

If there anything libstdc++, or maybe the middle man (Mesa in my case) can do,
to improve the situation.

Maybe if there is an API Mesa can call to "refresh" libstdc++'s memory about
pthread?

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #6 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
I meant to say "Is there anything ... can do, to improve the situation?"

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It will work when Glibc defines all the pthread functions in libc.so and
libpthread.so becomes empty. There's not really any way to make it work until
then that wouldn't pessimize programs that are entirely single-threaded.

You can use LD_PRELOAD=libpthread.so to "fix" the program by ensuring the
pthread symbols are present early, before any __cxa_guard_acquire calls happen.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm also going to start using Glibc's __libc_single_threaded instead of
__gthread_active_p() for deciding whether to use atomics for reference counting
in shared_ptr and the old std::string.

Once reference counting no longer depends on whether we're linked to libpthread
it won't be so costly to just link libstdc++.so to libpthread.so
unconditionally, so that std::thread Just Works without needing to use -pthread
and __gthread_active_p() is always true.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #9 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Jonathan Wakely from comment #7)
> It will work when Glibc defines all the pthread functions in libc.so and
> libpthread.so becomes empty.

Won't that mean libstdc++ will always use the thread-safe behavior? If that's
the goal, why don't we just remove the __gthread_active_p check?

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Because it pessimizes single threaded programs. After we've made some other
changes that depend on a recent glibc, then we can avoid that pessimization.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-08-27
             Status|UNCONFIRMED                 |NEW

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Something like this should work with the latest glibc:

--- a/libstdc++-v3/libsupc++/guard.cc
+++ b/libstdc++-v3/libsupc++/guard.cc
@@ -252,7 +252,22 @@ namespace __cxxabiv1
 # ifdef _GLIBCXX_USE_FUTEX
     // If __atomic_* and futex syscall are supported, don't use any global
     // mutex.
+#if __has_include(<sys/single_threaded.h>)
+    // Prefer to rely on __libc_single_threaded instead of __gthread_active_p
+    if (__libc_single_threaded)
+      {
+       int *gi = (int *) (void *) g;
+       if (*gi == 0)
+         {
+           *gi = _GLIBCXX_GUARD_PENDING_BIT;
+           return 1;
+         }
+       else
+         throw_recursive_init_exception();
+      }
+#else
     if (__gthread_active_p ())
+#endif
       {
        int *gi = (int *) (void *) g;
        const int guard_bit = _GLIBCXX_GUARD_BIT;
@@ -390,7 +405,17 @@ namespace __cxxabiv1
 #ifdef _GLIBCXX_USE_FUTEX
     // If __atomic_* and futex syscall are supported, don't use any global
     // mutex.
+#if __has_include(<sys/single_threaded.h>)
+    // Prefer to rely on __libc_single_threaded instead of __gthread_active_p
+    if (__libc_single_threaded)
+      {
+       int *gi = (int *) (void *) g;
+       *gi = _GLIBCXX_GUARD_BIT;
+       return;
+      }
+#else
     if (__gthread_active_p ())
+#endif
       {
        int *gi = (int *) (void *) g;
        const int guard_bit = _GLIBCXX_GUARD_BIT;

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oh plus this for the aborted init case:

@@ -351,7 +366,17 @@ namespace __cxxabiv1
 #ifdef _GLIBCXX_USE_FUTEX
     // If __atomic_* and futex syscall are supported, don't use any global
     // mutex.
+#if __has_include(<sys/single_threaded.h>)
+    // Prefer to rely on __libc_single_threaded instead of __gthread_active_p
+    if (__libc_single_threaded)
+      {
+       int *gi = (int *) (void *) g;
+       *gi = 0;
+       return;
+      }
+#else
     if (__gthread_active_p ())
+#endif
       {
        int *gi = (int *) (void *) g;
        const int waiting_bit = _GLIBCXX_GUARD_WAITING_BIT;

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Yuxuan Shui from comment #1)
> Example:
> 
> This program normally deadlocks when using linked pthread:
> 
> https://godbolt.org/z/Yrza4e
> 
> But it will throw recursive_init_error when using dlopen'd pthread:
> 
> https://godbolt.org/z/afMe5d

I'm not sure what these examples are very helpful. Throwing
recursive_init_error is the correct behaviour here, it's a bug that we fail to
detect the recursion when it happens in the same thread.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=97211

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #13)
> I'm not sure what these examples are very helpful. Throwing
> recursive_init_error is the correct behaviour here, it's a bug that we fail
> to detect the recursion when it happens in the same thread.

I created PR 97211 for that.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #15 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Jonathan Wakely from comment #13)
> (In reply to Yuxuan Shui from comment #1)
> > Example:
> > 
> > This program normally deadlocks when using linked pthread:
> > 
> > https://godbolt.org/z/Yrza4e
> > 
> > But it will throw recursive_init_error when using dlopen'd pthread:
> > 
> > https://godbolt.org/z/afMe5d
> 
> I'm not sure what these examples are very helpful. Throwing
> recursive_init_error is the correct behaviour here, it's a bug that we fail
> to detect the recursion when it happens in the same thread.

This is just to demonstrate the difference in the behavior. It's easy to
imagine this happen in a legitimate use case (and we indeed have one).

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[yshuiv7 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #16 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
But yeah, that's definitely a bug in itself as well.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:e6923541fae5081b646f240d54de2a32e17a0382

commit r11-3484-ge6923541fae5081b646f240d54de2a32e17a0382
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat Sep 26 20:32:36 2020 +0100

    libstdc++: Use __libc_single_threaded to optimise atomics [PR 96817]

    Glibc 2.32 adds a global variable that says whether the process is
    single-threaded. We can use this to decide whether to elide atomic
    operations, as a more precise and reliable indicator than
    __gthread_active_p.

    This means that guard variables for statics and reference counting in
    shared_ptr can use less expensive, non-atomic ops even in processes that
    are linked to libpthread, as long as no threads have been created yet.
    It also means that we switch to using atomics if libpthread gets loaded
    later via dlopen (this still isn't supported in general, for other
    reasons).

    We can't use __libc_single_threaded to replace __gthread_active_p
    everywhere. If we replaced the uses of __gthread_active_p in std::mutex
    then we would elide the pthread_mutex_lock in the code below, but not
    the pthread_mutex_unlock:

      std::mutex m;
      m.lock();            // pthread_mutex_lock
      std::thread t([]{}); // __libc_single_threaded = false
      t.join();
      m.unlock();          // pthread_mutex_unlock

    We need the lock and unlock to use the same "is threading enabled"
    predicate, and similarly for init/destroy pairs for mutexes and
    condition variables, so that we don't try to release resources that were
    never acquired.

    There are other places that could use __libc_single_threaded, such as
    _Sp_locker in src/c++11/shared_ptr.cc and locale init functions, but
    they can be changed later.

    libstdc++-v3/ChangeLog:

            PR libstdc++/96817
            * include/ext/atomicity.h (__gnu_cxx::__is_single_threaded()):
            New function wrapping __libc_single_threaded if available.
            (__exchange_and_add_dispatch, __atomic_add_dispatch): Use it.
            * libsupc++/guard.cc (__cxa_guard_acquire, __cxa_guard_abort)
            (__cxa_guard_release): Likewise.
            * testsuite/18_support/96817.cc: New test.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[rimvydas.jas at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

Rimvydas (RJ) <rimvydas.jas at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rimvydas.jas at gmail dot com

--- Comment #18 from Rimvydas (RJ) <rimvydas.jas at gmail dot com> ---
When libstdc++ is configured with --disable-linux-futex for use in valgrind,
the g:e6923541fae5081b646f240d54de2a32e17a0382 on x86_64-linux gives

WARNING: 18_support/96817.cc execution test program timed out.
FAIL: 18_support/96817.cc execution test

Also, disabling linux-futex fails libstdc++-abi/abi_check for symbols?
std::__atomic_futex_unsigned_base::_M_futex_wait_until()
std::__atomic_futex_unsigned_base::_M_futex_notify_all()

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #19 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I don't really care about test failures for non-standard configurations like
that.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:9c1125c121423a9948fa39e71ef89ba4059a2fad

commit r11-4723-g9c1125c121423a9948fa39e71ef89ba4059a2fad
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 4 15:24:47 2020 +0000

    libstdc++: Fix test failure with --disable-linux-futex

    As noted in PR 96817 this new test fails if the library is built without
    futexes. That's expected of course, but we might as well fail more
    obviously than a deadlock that eventually times out.

    libstdc++-v3/ChangeLog:

            * testsuite/18_support/96817.cc: Fail fail if the library is
            configured to not use futexes.

[Bug libstdc++/96817] __cxa_guard_acquire unsafe against dynamically loaded pthread

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96817

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The abi_check failures are of course expected if you configure with an option
that alters the library ABI.