https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21334
--- Comment #54 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Furthermore, if two threads call the non-const begin() concurrently on a shared
rep, they both want to make a new clone and release their reference to the old
rep. If they both allocate a new clone and store it to _M_dataplus._M_p then
one of those will be lost, resulting in a memory leak. If we wanted to make it
possible to make concurrent calls to non-const members like begin(), it should
be possible to do that in multiple threads!
So we would need to synchronize the installation of the new cloned string,
probably using compare-exchange there as well.
So maybe something like:
template<typename _CharT, typename _Traits, typename _Alloc>
void
basic_string<_CharT, _Traits, _Alloc>::
_M_leak_hard()
{
// No need to create a new copy of an empty string when a non-const
// reference/pointer/iterator into it is obtained. Modifying the
// trailing null character is undefined, so the ref/pointer/iterator
// is effectively const anyway.
if (this->empty())
return;
#if defined(__GTHREADS)
if (!__gnu_cxx::__is_single_threaded())
{
_CharT* __data = __atomic_load_n(&_M_dataplus._M_p,
__ATOMIC_ACQUIRE);
while (true)
{
_Rep* __rep = reinterpret_cast<_Rep*>(__data) - 1;
_Atomic_word __count = __atomic_load_n(&__rep->_M_refcount,
__ATOMIC_ACQUIRE);
if (__count > 0) // shared, make a copy
{
const allocator_type __a = get_allocator();
_CharT* __newp = __rep->_M_clone(__a, 0);
_Rep* __newrep = reinterpret_cast<_Rep*>(__newp) - 1;
__newrep->_M_set_leaked();
if (__atomic_compare_exchange_n(&_M_dataplus._M_p, &__data,
__newp, /* weak = */ true,
__ATOMIC_RELEASE,
__ATOMIC_RELAXED))
{
__rep->_M_dispose(__a);
return;
}
// The rep changed, release the new copy and try again.
__newrep->_M_dispose(__a);
}
else if (__count == 0) // not shared, change to leaked
{
if (__atomic_compare_exchange_n(&__rep->_M_refcount,
&__count,
-1, /* weak = */ true,
__ATOMIC_RELEASE,
__ATOMIC_RELAXED))
return;
if (__count == -1) // another thread already leaked it
return;
// Otherwise, the rep is now shared, restart the loop.
// Reload the data pointer first in case it was changed
// by a concurrent call to _M_leak_hard().
__data = __atomic_load_n(&_M_dataplus._M_p,
__ATOMIC_ACQUIRE);
}
else // already leaked
return;
}
}
#endif
if (_M_rep()->_M_is_shared())
_M_mutate(0, 0, 0);
_M_rep()->_M_set_leaked();
}
Which is much more complicated (and slower) than the current code for
_M_leak_hard().
But wait!
If we have a non-shared string (refcount == 0) and two threads concurrently
call begin() while a third thread copies the string into a short-lived
temporary, we can get a user-after-free. The first thread sees count == 0 but
before it can do the compare-exchange the third thread gets scheduled and makes
a copy that increments refcount to 1. Then the second thread sees refcount == 1
and so makes a new clone of the data, updating _M_dataplus._M_p to the new
string and decrementing the old string's refcount back to 0. Then the third
thread destroys its temporary object, decrementing refcount below zero and
freeing the original string data. Finally, the first thread gets scheduled
again and does compare-exchange on the old rep which was just freed. Boom.
So even if we make all accesses atomic, we still can't cope with concurrent
modifications of the object as would be needed to make begin(), end(), data()
etc. safe to call concurrently.
The bottom line is that concurrent accesses to the reference count can be made
safe, but not concurrent accesses to two separate locations (the reference
count in the heap-allocated string and the _M_dataplus._M_p pointer that refers
to it). And the copy-on-write operation that occurs when non-const begin() is
called needs to modify both locations.
A very different design would be needed to make that work.
