Skip to content

Commit

Permalink
Properly test allocator awareness of string
Browse files Browse the repository at this point in the history 
Did fix some bugs in string

* We were only allocating _Newcapacity in _Copy_assign. This lead to memory corruption
* We were not calling _Tidy_deallocate in _Move_assign which leaked memory of lhs
* We did not swap proxies in swap if one of the strings was in SSO. Breaks with stateful allocators
* We always performed work in swap even if self swapping
  • Loading branch information
@miscco
miscco committed Aug 27, 2021
1 parent a5e7657 commit de327ee
Show file tree
Hide file tree
Showing 2 changed files with 449 additions and 101 deletions.
97 changes: 47 additions & 50 deletions stl/inc/xstring
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2780,38 +2780,7 @@ public:
_Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal()));
_Tidy_init();
}
#endif // _HAS_CXX20

private:
_CONSTEXPR20 void _Move_assign(basic_string& _Right, _Equal_allocators) noexcept {
_Tidy_deallocate();
_Pocma(_Getal(), _Right._Getal());
_Take_contents(_Right);
}

_CONSTEXPR20 void _Move_assign(basic_string& _Right, _Propagate_allocators) noexcept {
if (_Getal() == _Right._Getal()) {
_Move_assign(_Right, _Equal_allocators{});
} else {
// intentionally slams into noexcept on OOM, TRANSITION, VSO-466800
_Mypair._Myval2._Orphan_all();
_Mypair._Myval2._Reload_proxy(
_GET_PROXY_ALLOCATOR(_Alty, _Getal()), _GET_PROXY_ALLOCATOR(_Alty, _Right._Getal()));
_Pocma(_Getal(), _Right._Getal());
_Take_contents(_Right);
}
}

_CONSTEXPR20 void _Move_assign(basic_string& _Right, _No_propagate_allocators) {
if (_Getal() == _Right._Getal()) {
_Move_assign(_Right, _Equal_allocators{});
} else {
assign(_Right._Mypair._Myval2._Myptr(), _Right._Mypair._Myval2._Mysize);
}
}

public:
#if _HAS_CXX20
_NODISCARD bool _Move_assign_from_buffer(_Elem* const _Right, const size_type _Size, const size_type _Res) {
// Move assign from a buffer, used exclusively by basic_stringbuf; returns _Large_string_engaged()
_Tidy_deallocate();
Expand Down Expand Up @@ -2857,11 +2826,30 @@ public:
#endif // _HAS_CXX20

_CONSTEXPR20 basic_string& operator=(basic_string&& _Right) noexcept(
noexcept(_Move_assign(_Right, _Choose_pocma<_Alty>{}))) {
if (this != _STD addressof(_Right)) {
_Move_assign(_Right, _Choose_pocma<_Alty>{});
!is_same_v<_Choose_pocma<_Alty>, _No_propagate_allocators>) {
if (this == _STD addressof(_Right)) {
return *this;
}

using _POCMA = _Choose_pocma<_Alty>;
if constexpr (is_same_v<_POCMA, _Propagate_allocators>) {
if (_Getal() != _Right._Getal()) {
// intentionally slams into noexcept on OOM, TRANSITION, VSO-466800
_Mypair._Myval2._Orphan_all();
_Mypair._Myval2._Reload_proxy(
_GET_PROXY_ALLOCATOR(_Alty, _Getal()), _GET_PROXY_ALLOCATOR(_Alty, _Right._Getal()));
}

} else if constexpr (is_same_v<_POCMA, _No_propagate_allocators>) {
if (_Getal() != _Right._Getal()) {
assign(_Right._Mypair._Myval2._Myptr(), _Right._Mypair._Myval2._Mysize);
return *this;
}
}

_Tidy_deallocate();
_Pocma(_Getal(), _Right._Getal());
_Take_contents(_Right);
return *this;
}

Expand All @@ -2871,7 +2859,7 @@ public:
}

private:
void _Memcpy_val_from(const basic_string& _Right) noexcept {
_CONSTEXPR20 void _Memcpy_val_from(const basic_string& _Right) noexcept {
_STL_INTERNAL_CHECK(_Can_memcpy_val); // TRANSITION, if constexpr
const auto _My_data_mem =
reinterpret_cast<unsigned char*>(_STD addressof(_Mypair._Myval2)) + _Memcpy_val_offset;
Expand Down Expand Up @@ -2914,6 +2902,11 @@ private:
_Right_data._Bx._Ptr = nullptr;
_Swap_proxy_and_iterators(_Right);
} else { // copy small string buffer
#if _HAS_CXX20
if (_STD is_constant_evaluated()) {
_Construct_in_place(_Mypair._Myval2._Bx);
}
#endif // _HAS_CXX20
_Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1);
_Right_data._Orphan_all();
}
Expand Down Expand Up @@ -3017,19 +3010,25 @@ public:
static constexpr auto npos{static_cast<size_type>(-1)};

private:
void _Copy_assign_val_from_small(const basic_string& _Right) {
_CONSTEXPR20 void _Copy_assign_val_from_small(const basic_string& _Right) {
// TRANSITION, VSO-761321; inline into only caller when that's fixed
_Tidy_deallocate();
if constexpr (_Can_memcpy_val) {
_Memcpy_val_from(_Right);
} else {
auto& _My_data = _Mypair._Myval2;
auto& _Right_data = _Right._Mypair._Myval2;

_Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1);
_My_data._Mysize = _Right_data._Mysize;
_My_data._Myres = _Right_data._Myres;
#if _HAS_CXX20
if (!is_constant_evaluated())
#endif // _HAS_CXX20
{
_Memcpy_val_from(_Right);
return;
}
}

auto& _My_data = _Mypair._Myval2;
auto& _Right_data = _Right._Mypair._Myval2;

_Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1);
_My_data._Mysize = _Right_data._Mysize;
_My_data._Myres = _Right_data._Myres;
}

_CONSTEXPR20 void _Copy_assign(const basic_string& _Right, false_type) {
Expand All @@ -3053,7 +3052,7 @@ private:
const auto _New_size = _Right._Mypair._Myval2._Mysize;
const auto _New_capacity = _Calculate_growth(_New_size, 0, _Right.max_size());
auto _Right_al_non_const = _Right_al;
const auto _New_ptr = _Right_al_non_const.allocate(_New_capacity); // throws
const auto _New_ptr = _Right_al_non_const.allocate(_New_capacity + 1); // throws

#if _HAS_CXX20
if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB
Expand Down Expand Up @@ -4100,13 +4099,11 @@ public:
_Right._Mypair._Myval2._Orphan_all();
}

if (_My_large || _Right_large) {
_Mypair._Myval2._Swap_proxy_and_iterators(_Right._Mypair._Myval2);
}
_Mypair._Myval2._Swap_proxy_and_iterators(_Right._Mypair._Myval2);
#endif // _ITERATOR_DEBUG_LEVEL != 0
}

_Swap_data(_Right);
_Swap_data(_Right);
}
}

#if _HAS_CXX17
Expand Down
Loading

0 comments on commit de327ee

Please sign in to comment.