[libcxx-commits] [libcxx] Reapply "[libc++] Fix incorrect size check when growing std::string" (#234545) (PR #234551)

[via libcxx-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

<details>
<summary>Changes</summary>

There was a race condition with another commit adding another call of
`__get_amortized_growth_capacity`. This is fixed in this version.

Original commit message:

When trying to grow very large amounts the `max_size()` check could fail due to
the result of adding the current capacity and the additional capacity could wrap.

This reverts commit 64b40b84eb293e259cfe36add2ac34ab3dafc05c.


---
Full diff: https://github.com/llvm/llvm-project/pull/234551.diff


4 Files Affected:

- (modified) libcxx/include/string (+9-9) 
- (modified) libcxx/test/std/strings/basic.string/string.modifiers/string_append/size_char.pass.cpp (+11) 
- (modified) libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_size_char.pass.cpp (+11) 
- (modified) libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp (+11) 


``````````diff
diff --git a/libcxx/include/string b/libcxx/include/string
index 33175c115fd10c..d264109a3d91ef 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -2306,16 +2306,16 @@ private:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
-  __get_amortized_growth_capacity(size_type __required_capacity) {
+  __get_amortized_growth_capacity(size_type __old_cap, size_type __additional_characters) {
     size_type __max_size = max_size();
-    if (__required_capacity > __max_size)
+    if (__additional_characters > __max_size - __old_cap)
       __throw_length_error();
     size_type __current_cap = capacity();
-    _LIBCPP_ASSERT_INTERNAL(
-        __current_cap < __required_capacity, "Trying to grow string even though there is enough capacity already?");
+    _LIBCPP_ASSERT_INTERNAL(__current_cap < __old_cap + __additional_characters,
+                            "Trying to grow string even though there is enough capacity already?");
     if (__current_cap > __max_size / 2 - __alignment)
       return __max_size;
-    return std::max(__required_capacity, 2 * __current_cap);
+    return std::max(__old_cap + __additional_characters, 2 * __current_cap);
   }
 
   inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void __init(const value_type* __s, size_type __sz);
@@ -2659,7 +2659,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
     size_type __n_del,
     size_type __n_add,
     const value_type* __p_new_stuff) {
-  __long __buffer     = __allocate_long_buffer(__alloc_, 0, __get_amortized_growth_capacity(__old_cap + __delta_cap));
+  __long __buffer     = __allocate_long_buffer(__alloc_, 0, __get_amortized_growth_capacity(__old_cap, __delta_cap));
   value_type* __old_p = __data();
   __annotate_delete();
   auto __guard = std::__make_scope_guard(__annotate_new_size(*this));
@@ -2691,7 +2691,7 @@ basic_string<_CharT, _Traits, _Allocator>::__grow_by(
     size_type __n_copy,
     size_type __n_del,
     size_type __n_add) {
-  __long __buffer     = __allocate_long_buffer(__alloc_, 0, __get_amortized_growth_capacity(__old_cap + __delta_cap));
+  __long __buffer     = __allocate_long_buffer(__alloc_, 0, __get_amortized_growth_capacity(__old_cap, __delta_cap));
   value_type* __old_p = __data();
   if (__n_copy != 0)
     traits_type::copy(std::__to_address(__buffer.__data_), __old_p, __n_copy);
@@ -2744,7 +2744,7 @@ basic_string<_CharT, _Traits, _Allocator>::__assign_c_str(const value_type* __s,
         if _LIBCPP_CONSTEXPR (!__is_short)
           __this.__rep_ = __rep();
       });
-      auto __new_cap = __this.__get_amortized_growth_capacity(__ln);
+      auto __new_cap = __this.__get_amortized_growth_capacity(0, __ln);
       std::__assume(__new_cap <= __this.max_size());
       __this.__init_internal_buffer(__new_cap);
       __this.__set_long_size(__ln);
@@ -2938,7 +2938,7 @@ basic_string<_CharT, _Traits, _Allocator>::__assign_trivial(_Iterator __first, _
   if (__cap < __n) {
     // __cap < __n does not mean the range is outside the string: it can be [data(), data() + size() + 1) of a full
     // buffer, or the bytes of the object itself. Copy into the new buffer before releasing the old one.
-    __long __buffer = __allocate_long_buffer(__alloc_, __n, __get_amortized_growth_capacity(__n));
+    __long __buffer = __allocate_long_buffer(__alloc_, __n, __get_amortized_growth_capacity(0, __n));
     auto __end      = __copy_non_overlapping_range(__first, __last, std::__to_address(__buffer.__data_));
     traits_type::assign(*__end, value_type());
 
diff --git a/libcxx/test/std/strings/basic.string/string.modifiers/string_append/size_char.pass.cpp b/libcxx/test/std/strings/basic.string/string.modifiers/string_append/size_char.pass.cpp
index a898efcf21855f..608c9dec5c328d 100644
--- a/libcxx/test/std/strings/basic.string/string.modifiers/string_append/size_char.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.modifiers/string_append/size_char.pass.cpp
@@ -63,5 +63,16 @@ int main(int, char**) {
   static_assert(test());
 #endif
 
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  { // Append with invalid parameters
+    std::string str = "Banane";
+    try {
+      str.append(std::string::npos, 'a');
+      assert(false);
+    } catch (const std::length_error&) {
+    }
+  }
+#endif
+
   return 0;
 }
diff --git a/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_size_char.pass.cpp b/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_size_char.pass.cpp
index 6937ccc0d540a4..9107939ed420ce 100644
--- a/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_size_char.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_size_char.pass.cpp
@@ -112,5 +112,16 @@ int main(int, char**) {
   static_assert(test());
 #endif
 
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  { // Append with invalid parameters
+    std::string str = "Banane";
+    try {
+      str.insert(str.begin(), std::string::npos, 'a');
+      assert(false);
+    } catch (const std::length_error&) {
+    }
+  }
+#endif
+
   return 0;
 }
diff --git a/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp b/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp
index 0023014976ec51..15a652fce07d24 100644
--- a/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp
@@ -143,5 +143,16 @@ int main(int, char**) {
   static_assert(test());
 #endif
 
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  { // Append with invalid parameters
+    std::string str = "Banane";
+    try {
+      str.insert(0, std::string::npos, 'a');
+      assert(false);
+    } catch (const std::length_error&) {
+    }
+  }
+#endif
+
   return 0;
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/234551