[libcxx-commits] [libcxx] [libcxx] Don't deallocate non-pointer data in string assignment. (PR #67200)

[James Y Knight via libcxx-commits]

https://github.com/jyknight created https://github.com/llvm/llvm-project/pull/67200

Previously, assignment to a std::basic_string type with a _custom_ allocator could under certain conditions attempt to interpret part of the target string's "short" string-content as if it was a "long" data pointer, and attempt to deallocate a garbage value.

This is a serious bug, but code in which it might happen is rare. It required:

1. the basic_string must be using a custom allocator type which sets the propagate_on_container_copy_assignment trait to true (thus, it does not affect the default allocator, nor most custom allocators).
2. the allocator for the target string must compare not equal to the allocator for the source string (many allocators always compare equal).
3. the source of the copy must currently contain a "long" string, and the assignment-target must currently contain a "short" string.

Finally, the issue would've typically been innocuous when the bytes misinterpreted as a pointer were all zero, as deallocating a nullptr is typically a no-op. This is why existing test cases did not exhibit an issue: they were all zero-length strings, which do not have data in the bytes interpreted as a pointer.

>From c4389a57972bfbdb43e4a00a6614ae3e5c983070 Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight at google.com>
Date: Fri, 22 Sep 2023 21:24:20 +0000
Subject: [PATCH] [libcxx] Don't deallocate a non-pointer data in string
 assignment.

Previously, assignment to a std::basic_string type with a _custom_
allocator could under certain conditions attempt to interpret part of
the target string's "short" string-content as if it was a "long" data
pointer, and attempt to deallocate a garbage value.

This is a serious bug, but code in which it might happen is rare. It required:

1. the basic_string must be using a custom allocator type which sets
   the propagate_on_container_copy_assignment trait to true (thus, it
   does not affect the default allocator, nor most custom allocators).
2. the allocator for the target string must compare not equal to the
   allocator for the source string (many allocators always compare
   equal).
3. the source of the copy must currently contain a "long" string, and
   the assignment-target must currently contain a "short" string.

Finally, the issue would've typically been innocuous when the bytes misinterpreted as a pointer were all zero, as deallocating a nullptr is typically a no-op. This is why existing test cases did not exhibit an issue: they were all zero-length strings, which do not have data in the bytes interpreted as a pointer.
---
 libcxx/include/string                                          | 3 ++-
 .../string.modifiers/string_assign/string.pass.cpp             | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libcxx/include/string b/libcxx/include/string
index 4b96273698166dd..123e1d64f910ab7 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1969,7 +1969,8 @@ private:
                     allocator_type __a = __str.__alloc();
                     auto __allocation = std::__allocate_at_least(__a, __str.__get_long_cap());
                     __begin_lifetime(__allocation.ptr, __allocation.count);
-                    __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
+                    if (__is_long())
+                        __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
                     __alloc() = std::move(__a);
                     __set_long_pointer(__allocation.ptr);
                     __set_long_cap(__allocation.count);
diff --git a/libcxx/test/std/strings/basic.string/string.modifiers/string_assign/string.pass.cpp b/libcxx/test/std/strings/basic.string/string.modifiers/string_assign/string.pass.cpp
index 311778c35dcbaa9..6db086e78ed0f40 100644
--- a/libcxx/test/std/strings/basic.string/string.modifiers/string_assign/string.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.modifiers/string_assign/string.pass.cpp
@@ -90,6 +90,7 @@ TEST_CONSTEXPR_CXX20 bool test() {
     testAlloc(S(A(5)), S("1"), A());
     testAlloc(S(A(5)), S("1", A(7)), A(7));
     testAlloc(S(A(5)), S("1234567890123456789012345678901234567890123456789012345678901234567890", A(7)), A(7));
+    testAlloc(S("12345678901234567890", A(5)), S("1234567890123456789012345678901234567890123456789012345678901234567890", A(7)), A(7));
   }
 
 #if TEST_STD_VER >= 11