Re: [musl] Mutexes are not unlocking

[Jonathan Wakely <jwakely@redhat.com>]

On 22/11/20 14:28 -0500, Rich Felker wrote:
>On Sun, Nov 22, 2020 at 08:23:23PM +0100, Florian Weimer wrote:
>> * Rich Felker:
>>
>> > On Sun, Nov 22, 2020 at 09:43:35PM +0300, Арсений wrote:
>> >>
>> >> Hello,
>> >> The problem is that mutex is not got unlocked after the first unlock().
>> >>  
>> >> libstdc++ uses a wrapper for pthread called gthreads. This wrapper
>> >> checks for the state of the mutex system. For
>> >> example, pthread_mutex_unlock() is called in a following way:
>> >>  
>> >> static inline int
>> >> __gthread_mutex_unlock (__gthread_mutex_t *__mutex)
>> >> {
>> >>   if (__gthread_active_p ())
>> >>     return __gthrw_(pthread_mutex_unlock) (__mutex);
>> >>   else
>> >>     return 0;
>> >> }
>> >
>> > Yes. This code is invalid (it misinterprets weak symbol information to
>> > draw incorrect conclusions about whether threads may be in use) and
>> > thus is disabled in builds of gcc/libstdc++ targeting musl-based
>> > systems. GCC and glibc-based distro folks mostly don't care because
>> > it only breaks static linking, but some of them actually hack gcc's
>> > libpthread.a into one giant .o file to work around the problem rather
>> > than fixing this in gcc...
>>
>> GCC 11 has a fix (if used along with glibc 2.32), but I wonder if it's

The GCC 11 change isn't used for mutexes. I use __libc_single_threaded
for deciding whether to use atomics or plain non-atomic ops for
ref-count updates. But for actions like mutex locks I don't use it,
because it would do the wrong thing for code like:

int main() {
   std::mutex m;
   std::lock_guard<std::mutex> l(m);
   std::thread t( []{} };
   t.join();
}

In this program __libc_single_threaded is true when we construct the
lock_guard, but false when it is destroyed at the end of the block. If
we checked __libc_single_threaded for mutex locking/unlocking then we
would be inconsistent here, we would elide the lock but try to unlock
a mutex that was never locked, which would be UB.

Whatever condition we check to decide whether to call the pthreads
function needs to give the same result for the lock and unlock. That
condition is currently __gthread_active_p, which checks if a symbol is
weak.

>> going to run into a similar issue because inlined code from older GCC
>> versions uses a diverging version of the check.

No, everything uses the same check. GCC 11 didn't change anything for
mutexes.

>> Jonathan, more context is here:
>>
>>   <https://www.openwall.com/lists/musl/2020/11/22/1>
>
>Uhg, that's a really nasty problem. Is __gthread_active_p() also
>inlined? Or can it be given a definition to mitigate the problem?

It's inlined, and making it a non-inline PLT call would negate some of
the point of using it (it's there mostly as an optimization).

But I don't think inlining it is a problem, because its definition
hasn't changed.

I think the right thing to do is a new configuration which completely
avoids using weak symbols in gthreads, and just calls the pthread
symbols directly. A future version of glibc is going to move pthreads
symbols into libc anyway:
https://sourceware.org/legacy-ml/libc-alpha/2019-10/msg00080.html
After that, the __gthread_active_p condition is just a wasted branch,
because it will be unconditionally true:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89714

If the musl libstdc++ removes the use of __gthread_active_p, but code
compiled against a glibc libstdc++ inlines the __gthread_active_p
check, then it seems to me that musl (or the musl libstdc++) needs to
ensure that the inlined __gthread_active_p will return true. It can do
that by providing a non-weak symbol called __pthread_key_create (the
symbol won't be called, it just has to exist).