[libcxx-commits] [libcxx] [libc++][ranges][abi-break] Fix `movable_box` overwriting memory of data that lives in the tail padding (PR #71314)

[via libcxx-commits]

https://github.com/huixie90 created https://github.com/llvm/llvm-project/pull/71314

None

>From bc81245ad3c55560752b6bf4637e96be7db2e64c Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Sun, 5 Nov 2023 10:48:04 +0000
Subject: [PATCH] [libc++][ranges][abi-break] Fix `movable_box` overwriting
 memory of data that lives in the tail padding

---
 libcxx/include/__ranges/movable_box.h         | 45 ++++++++++++++-----
 .../no_unique_address.pass.cpp                | 14 ++++++
 .../range.transform/general.pass.cpp          | 17 +++++++
 3 files changed, 64 insertions(+), 12 deletions(-)

diff --git a/libcxx/include/__ranges/movable_box.h b/libcxx/include/__ranges/movable_box.h
index 6615533d374349e..13e28954fd862f9 100644
--- a/libcxx/include/__ranges/movable_box.h
+++ b/libcxx/include/__ranges/movable_box.h
@@ -16,6 +16,7 @@
 #include <__config>
 #include <__memory/addressof.h>
 #include <__memory/construct_at.h>
+#include <__type_traits/is_empty.h>
 #include <__type_traits/is_nothrow_constructible.h>
 #include <__type_traits/is_nothrow_copy_constructible.h>
 #include <__type_traits/is_nothrow_default_constructible.h>
@@ -146,21 +147,41 @@ template <class _Tp>
 concept __doesnt_need_empty_state = __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>;
 #  endif
 
+template <class _Tp, bool _NoUniqueAddress>
+struct __no_unique_address_if {
+  _Tp __val_;
+
+  template <class... _Args>
+    requires is_constructible_v<_Tp, _Args&&...>
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+      : __val_(std::forward<_Args>(__args)...) {}
+};
+
+template <class _Tp>
+struct __no_unique_address_if<_Tp, true> {
+  _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
+
+  template <class... _Args>
+    requires is_constructible_v<_Tp, _Args&&...>
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+      : __val_(std::forward<_Args>(__args)...) {}
+};
+
 template <__movable_box_object _Tp>
   requires __doesnt_need_empty_state<_Tp>
-class __movable_box<_Tp> {
-  _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
+class __movable_box<_Tp> : private __no_unique_address_if<_Tp, is_empty_v<_Tp>> {
+  using __base = __no_unique_address_if<_Tp, is_empty_v<_Tp>>;
 
 public:
   template <class... _Args>
     requires is_constructible_v<_Tp, _Args...>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box(in_place_t, _Args&&... __args) noexcept(
       is_nothrow_constructible_v<_Tp, _Args...>)
-      : __val_(std::forward<_Args>(__args)...) {}
+      : __base(std::forward<_Args>(__args)...) {}
 
   _LIBCPP_HIDE_FROM_ABI constexpr __movable_box() noexcept(is_nothrow_default_constructible_v<_Tp>)
     requires default_initializable<_Tp>
-      : __val_() {}
+      : __base() {}
 
   _LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box const&) = default;
   _LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box&&)      = default;
@@ -177,8 +198,8 @@ class __movable_box<_Tp> {
   _LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box const& __other) noexcept {
     static_assert(is_nothrow_copy_constructible_v<_Tp>);
     if (this != std::addressof(__other)) {
-      std::destroy_at(std::addressof(__val_));
-      std::construct_at(std::addressof(__val_), __other.__val_);
+      std::destroy_at(std::addressof(this->__val_));
+      std::construct_at(std::addressof(this->__val_), __other.__val_);
     }
     return *this;
   }
@@ -186,17 +207,17 @@ class __movable_box<_Tp> {
   _LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box&& __other) noexcept {
     static_assert(is_nothrow_move_constructible_v<_Tp>);
     if (this != std::addressof(__other)) {
-      std::destroy_at(std::addressof(__val_));
-      std::construct_at(std::addressof(__val_), std::move(__other.__val_));
+      std::destroy_at(std::addressof(this->__val_));
+      std::construct_at(std::addressof(this->__val_), std::move(__other.__val_));
     }
     return *this;
   }
 
-  _LIBCPP_HIDE_FROM_ABI constexpr _Tp const& operator*() const noexcept { return __val_; }
-  _LIBCPP_HIDE_FROM_ABI constexpr _Tp& operator*() noexcept { return __val_; }
+  _LIBCPP_HIDE_FROM_ABI constexpr _Tp const& operator*() const noexcept { return this->__val_; }
+  _LIBCPP_HIDE_FROM_ABI constexpr _Tp& operator*() noexcept { return this->__val_; }
 
-  _LIBCPP_HIDE_FROM_ABI constexpr const _Tp* operator->() const noexcept { return std::addressof(__val_); }
-  _LIBCPP_HIDE_FROM_ABI constexpr _Tp* operator->() noexcept { return std::addressof(__val_); }
+  _LIBCPP_HIDE_FROM_ABI constexpr const _Tp* operator->() const noexcept { return std::addressof(this->__val_); }
+  _LIBCPP_HIDE_FROM_ABI constexpr _Tp* operator->() noexcept { return std::addressof(this->__val_); }
 
   _LIBCPP_HIDE_FROM_ABI constexpr bool __has_value() const noexcept { return true; }
 };
diff --git a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
index e21191f98dea3e8..6a33dab96280afa 100644
--- a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
+++ b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
@@ -15,6 +15,20 @@
 #include <cassert>
 #include <utility>
 
+// movable_box should not use no_unique_address internally if T is not empty
+struct WithPadding {
+  WithPadding() {}
+  int i;
+  bool b;
+};
+
+struct Test {
+  [[no_unique_address]] std::ranges::__movable_box<WithPadding> box_;
+  bool b2;
+};
+
+static_assert(sizeof(Test) > sizeof(std::ranges::__movable_box<WithPadding>));
+
 bool copied = false;
 bool moved  = false;
 
diff --git a/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
index 32fed6f8caa7d02..cfe394e89183238 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
@@ -108,5 +108,22 @@ int main(int, char**) {
   }
 #endif
 
+  // GH issue #70506
+  // movable_box::operator= overwrites underlying view
+  {
+    auto f = [l = 0.0L, b = false](int i) {
+      (void)l;
+      (void)b;
+      return i;
+    };
+
+    auto v1 = std::vector{1, 2, 3, 4} | std::views::transform(f);
+    auto v2 = std::vector{1, 2, 3, 4} | std::views::transform(f);
+
+    v1             = std::move(v2);
+    int expected[] = {1, 2, 3, 4};
+    assert(std::equal(v1.begin(), v1.end(), expected, expected + 4));
+  }
+
   return 0;
 }