On Mon, 8 Sept 2025 at 14:38, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Mon, Sep 8, 2025 at 2:45 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> This adds checks when incrementing the shared count and weak count and
>> will trap if they would be
>> be incremented past its maximum. The maximum value is the value at which
>> incrementing it produces an invalid use_count(). So that is either the
>> maximum positive value of _Atomic_word, or for targets where we now
>> allow the counters to wrap around to negative values, the "maximum"
>> value is -1, because that is the value at which one more increment
>> overflows the usable range and resets the counter to zero.
>>
>> libstdc++-v3/ChangeLog:
>>
>> PR libstdc++/71945
>> * include/bits/shared_ptr_base.h (_Sp_counted_base::_S_chk):
>> Trap if a reference count cannot be incremented any higher.
>> (_Sp_counted_base::_M_add_ref_copy): Use _S_chk.
>> (_Sp_counted_base::_M_add_weak_ref): Likewise.
>> (_Sp_counted_base<_S_mutex>::_M_add_ref_lock_nothrow): Likewise.
>> (_Sp_counted_base<_S_atomic>::_M_add_ref_lock_nothrow): Likewise.
>> (_Sp_counted_base<_S_single>::_M_add_ref_copy): Use _S_chk.
>> ---
>>
>> v2: The original patch series only had two patches, but the
>> implementation of trapping on counter overflow has been split out from
>> PATCH 2/2 into a third patch.
>
> If the use of max in _M_weak_add_ref is simple type, then this is OK for trunk
> with the change. Otherwise, I think I do not understand the patches.
>>
>>
>> We should decide whether we want to do these checks and traps
>> unconditionally, or only for _GLIBCXX_ASSERTIONS.
>>
>> Tested powerpc64le-linux.
>>
>> libstdc++-v3/include/bits/shared_ptr_base.h | 51 +++++++++++++++++++--
>> 1 file changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h
>> b/libstdc++-v3/include/bits/shared_ptr_base.h
>> index 2ea53ae15998..a636e3e30ba5 100644
>> --- a/libstdc++-v3/include/bits/shared_ptr_base.h
>> +++ b/libstdc++-v3/include/bits/shared_ptr_base.h
>> @@ -148,7 +148,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> // Increment the use count (used when the count is greater than zero).
>> void
>> _M_add_ref_copy()
>> - { __gnu_cxx::__atomic_add_dispatch(&_M_use_count, 1); }
>> + { _S_chk(__gnu_cxx::__exchange_and_add_dispatch(&_M_use_count, 1)); }
>>
>> // Increment the use count if it is non-zero, throw otherwise.
>> void
>> @@ -200,7 +200,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> // Increment the weak count.
>> void
>> _M_weak_add_ref() noexcept
>> - { __gnu_cxx::__atomic_add_dispatch(&_M_weak_count, 1); }
>> + {
>> + // _M_weak_count can always use negative values because it cannot be
>> + // observed by users (unlike _M_use_count). See _S_chk for details.
>> + constexpr _Atomic_word __max
>> + = __gnu_cxx::__int_traits<_Atomic_word>::__max;
>
> Should this be -1, the comment says we can always use negative values for
> _M_weak_count.
Yes, just a brain-failure. The v1 patch had:
+ // _M_weak_count can always use negative values (except for _S_single)
+ // because only _M_use_count can be observed. See _M_chk for details.
+ constexpr _Atomic_word __max = _Lp != _S_single
+ ? -1 : __gnu_cxx::__int_traits<_Atomic_word>::__max;
and when I made _S_single consistent in the v2 patch I must have
deleted the wrong branch of the conditional-expression. Doh.
>>
>> +
>> + if (__gnu_cxx::__exchange_and_add_dispatch(&_M_weak_count, 1) ==
>> __max)
>> + [[__unlikely__]] __builtin_trap();
>> + }
>>
>> // Decrement the weak count.
>> void
>> @@ -238,6 +246,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> _Sp_counted_base(_Sp_counted_base const&) = delete;
>> _Sp_counted_base& operator=(_Sp_counted_base const&) = delete;
>>
>> + // Called when incrementing _M_use_count to cause a trap on overflow.
>> + // This should be passed the value of the counter before the
>> increment.
>> + static void
>> + _S_chk(_Atomic_word __count)
>> + {
>> + // __max is the maximum allowed value for the shared reference count.
>> + // All valid reference count values need to fit into [0,LONG_MAX)
>> + // because users can observe the count via shared_ptr::use_count().
>> + //
>> + // When long is wider than _Atomic_word, _M_use_count can go negative
>> + // and the cast in _Sp_counted_base::use_count() will turn it into a
>> + // positive value suitable for returning to users. The implementation
>> + // only cares whether _M_use_count reaches zero after a decrement,
>> + // so negative values are not a problem internally.
>> + // So when possible, use -1 for __max (incrementing past that would
>> + // overflow _M_use_count to 0, which means an empty shared_ptr).
>> + //
>> + // When long is not wider than _Atomic_word, __max is just the type's
>> + // maximum positive value. We cannot use negative counts because they
>> + // would not fit in [0,LONG_MAX) after casting to an unsigned type,
>> + // which would cause use_count() to return bogus values.
>> + constexpr _Atomic_word __max
>> + = sizeof(long) > sizeof(_Atomic_word)
>> + ? -1 : __gnu_cxx::__int_traits<_Atomic_word>::__max;
>> +
>> + if (__count == __max) [[__unlikely__]]
>> + __builtin_trap();
>> + }
>> +
>> _Atomic_word _M_use_count; // #shared
>> _Atomic_word _M_weak_count; // #weak + (#shared != 0)
>> };
>> @@ -248,7 +285,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> template<>
>> inline void
>> _Sp_counted_base<_S_single>::_M_add_ref_copy()
>> - { __gnu_cxx::__atomic_add_single(&_M_use_count, 1); }
>> + {
>> + _S_chk(_M_use_count);
>> + __gnu_cxx::__atomic_add_single(&_M_use_count, 1);
>> + }
>>
>> template<>
>> inline void
>> @@ -284,7 +324,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> _M_add_ref_lock_nothrow() noexcept
>> {
>> __gnu_cxx::__scoped_lock sentry(*this);
>> - if (__gnu_cxx::__exchange_and_add_dispatch(&_M_use_count, 1) == 0)
>> + if (auto __c = __gnu_cxx::__exchange_and_add_dispatch(&_M_use_count,
>> 1))
>> + _S_chk(__c);
>> + else
>> {
>> // Count was zero, so we cannot lock it to get a shared_ptr.
>> // Reset to zero. This isn't racy, because there are no shared_ptr
>> @@ -314,6 +356,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> while (!__atomic_compare_exchange_n(&_M_use_count, &__count, __count
>> + 1,
>> true, __ATOMIC_ACQ_REL,
>> __ATOMIC_RELAXED));
>> + _S_chk(__count);
>> return true;
>> }
>>
>> --
>> 2.51.0
>>
