[PATCH v2] libstdc++: Make atomic<shared_ptr<T>>::wait sensitive to stored pointer only changes [PR118757]

Tue Oct 14 12:22:06 GMT 2025

On Tue, 14 Oct 2025 at 13:53 +0200, Tomasz Kaminski wrote:
>On Tue, Oct 14, 2025 at 12:38 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>
>> On Thu, 21 Aug 2025 at 08:22 +0200, Tomasz Kamiński wrote:
>> >Previously, atomic<shared_ptr<T>>::wait (and the weak_ptr version) was
>> >equivalent to waiting directly on _M_val, which corresponds to the
>> pointer to
>> >the control block (_M_pi). Consequently, wakeups were not triggered if
>> the stored
>> >pointer value was changed to a pointer that uses the same control block
>> but
>> >stores pointer to a different object. Such a pointer can be constructed
>> using an
>> >aliasing constructor.
>> >
>> >To address this, wait now uses a generic proxy wait
>> std::__atomic_wait_address
>> >function, which supports waiting until any predicate is satisfied. The
>> provided
>> >predicate now compares both the control block (_M_pi) and the stored
>> pointer
>> >(_M_ptr). Comparing the latter requires locking the pointer.
>> >
>> >Since this function operates on raw pointers, the type of _M_val was
>> changed
>> >from __atomic_base<uintptr_t> to uninptr. Invocations of the corresponding
>>
>> s/uninptr/uintptr_t/
>>
>> >member functions are now replaced with direct use of __atomic builtins.
>>
>> The text above doesn't fit in 80 columns, and will look even worse in
>> the output of 'git log' where it's indented by another 4 columns.
>>
>> See https://cbea.ms/git-commit/#wrap-72 for recommendations on commit
>> message formatting.
>>
>>
>> >
>> >       PR libstdc++/118757
>> >
>> >libstdc++-v3/ChangeLog:
>> >
>> >       * include/bits/shared_ptr_atomic.h (_Atomic_count::_M_wait_unlock):
>> >       Add parameter capturing reference to _M_ptr. Reimplement in terms
>> >       of __atomic_wait_address.
>> >       (_Atomic_count::~_Atomic_count, _Atomic_count::lock)
>> >       (_Atomic_count::unlock, _Atomic_count::_M_swap_unlock): Replace
>> >       invocation of atomic member funcitons with __atomic builtins.
>> >       (_Atomic_count::notify_one, _Atomic_count::notify_all):
>> >       Use __atomic_notify_address.
>> >       (_Sp_atomic::element_type): Define.
>> >       (_Sp_atomic::_M_val): Change type to uintptr_t.
>> >       (_Sp_atomic::wait): Pass _M_ptr to _M_wait_unlock.
>> >       * python/libstdcxx/v6/printers.py:
>> >       * testsuite/20_util/shared_ptr/atomic/pr118757.cc: New test.
>> >       * testsuite/20_util/weak_ptr/pr118757.cc: New test.
>> >
>> >Signed-off-by: Tomasz Kamiński <tkaminsk@redhat.com>
>> >---
>> >Changes in v2:
>> > * update pretty printers
>> > * use __atomic_sub_fetch in _M_wait_unlock
>> >
>> >OK for trunk?
>> >As atomic wait fixes are in v16, not planning to backport this one.
>> >
>> > libstdc++-v3/include/bits/shared_ptr_atomic.h | 60 +++++++++++++------
>> > libstdc++-v3/python/libstdcxx/v6/printers.py  |  2 +-
>> > .../20_util/shared_ptr/atomic/pr118757.cc     | 46 ++++++++++++++
>> > .../testsuite/20_util/weak_ptr/pr118757.cc    | 47 +++++++++++++++
>> > 4 files changed, 137 insertions(+), 18 deletions(-)
>> > create mode 100644
>> libstdc++-v3/testsuite/20_util/shared_ptr/atomic/pr118757.cc
>> > create mode 100644 libstdc++-v3/testsuite/20_util/weak_ptr/pr118757.cc
>> >
>> >diff --git a/libstdc++-v3/include/bits/shared_ptr_atomic.h
>> b/libstdc++-v3/include/bits/shared_ptr_atomic.h
>> >index cc7841a8775..5f1e1b50514 100644
>> >--- a/libstdc++-v3/include/bits/shared_ptr_atomic.h
>> >+++ b/libstdc++-v3/include/bits/shared_ptr_atomic.h
>> >@@ -392,6 +392,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >     class _Sp_atomic
>> >     {
>> >       using value_type = _Tp;
>> >+      using element_type = typename _Tp::element_type;
>> >
>> >       friend struct atomic<_Tp>;
>> >
>> >@@ -420,7 +421,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >
>> >       ~_Atomic_count()
>> >       {
>> >-        auto __val = _M_val.load(memory_order_relaxed);
>> >+        auto __val = __atomic_load_n(&_M_val, int(memory_order_relaxed));
>>
>> Would it be cleaner to use the __atomic_impl::xxx wrappers (or just
>> use std::atomic_ref) instead of the raw __atomic_xxx built-ins?
>>
>> You'd still need to use __atomic_wait_address directly, but everything
>> else could use the nicer API, and not need to cast memory_order to
>> int.
>>
>I used __atomic_ref that is defined in bits/atomic_base.h class.
>Also added alignas(_AtomicRef::required_alignment) to _M_val.
>
>
>>
>> >         _GLIBCXX_TSAN_MUTEX_DESTROY(&_M_val);
>> >         __glibcxx_assert(!(__val & _S_lock_bit));
>> >         if (auto __pi = reinterpret_cast<pointer>(__val))
>>
>> Unrelated to this patch, but I wonder if we should do:
>>
>>    __val = __val & ~_S_lock_bit;
>>
>> here, so that if the destructor _does_ get run while it's locked (and
>> assertions are disabled), we don't get a segfault or bus error for the
>> unaligned pointer access.
>>
>I am not sure here if I would not prefer to get crash close to the problem
>when the date race was occuring.

Yes. Users who want memory safety should be enabling the assertions,
and then the destructor will abort if it's locked.

We could consider checking and using __builtin_trap() when assertions
are disabled, but that's a decision for another day.

>>
>> That would mean potentially decrementing the refcount while another
>> thread thinks it has it locked, which is still bad, and still a memory
>> safety issue.
>>
>In particular this thread could make a copy of it and return it.
>
>>
>> The safest thing to do here would be to acquire the lock in the
>> destructor, but that would add overhead for correct programs.
>>
>>