[libc++] Correctly handle custom deleters in hardened unique_ptr<T[]> (#110685)

It turns out that we can never do bounds-checking for unique_ptrs with
custom deleters, except when converting from a unique_ptr with a default
deleter to one with a custom deleter.

If we had an API like `std::make_unique` that allowed passing a custom
deleter, we could at least get bounds checking when the unique_ptr is
created through those APIs, but for now that is not possible.

Fixes #110683

Author: ldionne

libcxx/include/__memory/array_cookie.h

@@ -24,7 +24,7 @@
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // Trait representing whether a type requires an array cookie at the start of its allocation when
-// allocated as `new T[n]` and deallocated as `delete array`.
+// allocated as `new T[n]` and deallocated as `delete[] array`.
 //
 // Under the Itanium C++ ABI [1], we know that an array cookie is available unless `T` is trivially
 // destructible and the call to `operator delete[]` is not a sized operator delete. Under ABIs other

libcxx/include/__memory/unique_ptr.h

@@ -101,6 +101,12 @@ struct _LIBCPP_TEMPLATE_VIS default_delete<_Tp[]> {
   }
 };
 
+template <class _Deleter>
+struct __is_default_deleter : false_type {};
+
+template <class _Tp>
+struct __is_default_deleter<default_delete<_Tp> > : true_type {};
+
 template <class _Deleter>
 struct __unique_ptr_deleter_sfinae {
   static_assert(!is_reference<_Deleter>::value, "incorrect specialization");
@@ -307,11 +313,16 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
 // 1. When an array cookie (see [1]) exists at the beginning of the array allocation, we are
 //    able to reuse that cookie to extract the size of the array and perform bounds checking.
 //    An array cookie is a size inserted at the beginning of the allocation by the compiler.
-//    That size is inserted implicitly when doing `new T[n]` in some cases, and its purpose
-//    is to allow the runtime to destroy the `n` array elements when doing `delete array`.
+//    That size is inserted implicitly when doing `new T[n]` in some cases (as of writing this
+//    exactly when the array elements are not trivially destructible), and its main purpose is
+//    to allow the runtime to destroy the `n` array elements when doing `delete[] array`.
 //    When we are able to use array cookies, we reuse information already available in the
 //    current runtime, so bounds checking does not require changing libc++'s ABI.
 //
+//    However, note that we cannot assume the presence of an array cookie when a custom deleter
+//    is used, because the unique_ptr could have been created from an allocation that wasn't
+//    obtained via `new T[n]` (since it may not be deleted with `delete[] arr`).
+//
 // 2. When the "bounded unique_ptr" ABI configuration (controlled by `_LIBCPP_ABI_BOUNDED_UNIQUE_PTR`)
 //    is enabled, we store the size of the allocation (when it is known) so we can check it when
 //    indexing into the `unique_ptr`. That changes the layout of `std::unique_ptr<T[]>`, which is
@@ -328,7 +339,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
 //    try to fall back to using an array cookie when available.
 //
 //    Finally, note that when this ABI configuration is enabled, we have no choice but to always
-//    make space for a size to be stored in the unique_ptr. Indeed, while we might want to avoid
+//    make space for the size to be stored in the unique_ptr. Indeed, while we might want to avoid
 //    storing the size when an array cookie is available, knowing whether an array cookie is available
 //    requires the type stored in the unique_ptr to be complete, while unique_ptr can normally
 //    accommodate incomplete types.
@@ -339,7 +350,9 @@ struct __unique_ptr_array_bounds_stateless {
   __unique_ptr_array_bounds_stateless() = default;
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __unique_ptr_array_bounds_stateless(size_t) {}
 
-  template <class _Tp, __enable_if_t<__has_array_cookie<_Tp>::value, int> = 0>
+  template <class _Deleter,
+            class _Tp,
+            __enable_if_t<__is_default_deleter<_Deleter>::value && __has_array_cookie<_Tp>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Tp* __ptr, size_t __index) const {
     // In constant expressions, we can't check the array cookie so we just pretend that the index
     // is in-bounds. The compiler catches invalid accesses anyway.
@@ -349,7 +362,9 @@ struct __unique_ptr_array_bounds_stateless {
     return __index < __cookie;
   }
 
-  template <class _Tp, __enable_if_t<!__has_array_cookie<_Tp>::value, int> = 0>
+  template <class _Deleter,
+            class _Tp,
+            __enable_if_t<!__is_default_deleter<_Deleter>::value || !__has_array_cookie<_Tp>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Tp*, size_t) const {
     return true; // If we don't have an array cookie, we assume the access is in-bounds
   }
@@ -365,7 +380,9 @@ struct __unique_ptr_array_bounds_stored {
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __unique_ptr_array_bounds_stored(size_t __size) : __size_(__size) {}
 
   // Use the array cookie if there's one
-  template <class _Tp, __enable_if_t<__has_array_cookie<_Tp>::value, int> = 0>
+  template <class _Deleter,
+            class _Tp,
+            __enable_if_t<__is_default_deleter<_Deleter>::value && __has_array_cookie<_Tp>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Tp* __ptr, size_t __index) const {
     if (__libcpp_is_constant_evaluated())
       return true;
@@ -374,7 +391,9 @@ struct __unique_ptr_array_bounds_stored {
   }
 
   // Otherwise, fall back on the stored size (if any)
-  template <class _Tp, __enable_if_t<!__has_array_cookie<_Tp>::value, int> = 0>
+  template <class _Deleter,
+            class _Tp,
+            __enable_if_t<!__is_default_deleter<_Deleter>::value || !__has_array_cookie<_Tp>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Tp*, size_t __index) const {
     return __index < __size_;
   }
@@ -562,7 +581,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator[](size_t __i) const {
-    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__checker_.__in_bounds(std::__to_address(__ptr_), __i),
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__checker_.__in_bounds<deleter_type>(std::__to_address(__ptr_), __i),
                                         "unique_ptr<T[]>::operator[](index): index out of range");
     return __ptr_[__i];
   }

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp

@@ -48,31 +48,24 @@ struct MyDeleter {
 
 template <class WithCookie, class NoCookie>
 void test() {
-  // For types with an array cookie, we can always detect OOB accesses.
+  // For types with an array cookie, we can always detect OOB accesses. Note that reliance on an array
+  // cookie is limited to the default deleter, since a unique_ptr with a custom deleter may not have
+  // been allocated with `new T[n]`.
   {
-    // Check with the default deleter
     {
-      {
-        std::unique_ptr<WithCookie[]> ptr(new WithCookie[5]);
-        TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr<T[]>::operator[](index): index out of range");
-      }
-      {
-        std::unique_ptr<WithCookie[]> ptr = std::make_unique<WithCookie[]>(5);
-        TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr<T[]>::operator[](index): index out of range");
-      }
-#if TEST_STD_VER >= 20
-      {
-        std::unique_ptr<WithCookie[]> ptr = std::make_unique_for_overwrite<WithCookie[]>(5);
-        TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = WithCookie(), "unique_ptr<T[]>::operator[](index): index out of range");
-      }
-#endif
+      std::unique_ptr<WithCookie[]> ptr(new WithCookie[5]);
+      TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr<T[]>::operator[](index): index out of range");
     }
-
-    // Check with a custom deleter
     {
-      std::unique_ptr<WithCookie[], MyDeleter> ptr(new WithCookie[5]);
+      std::unique_ptr<WithCookie[]> ptr = std::make_unique<WithCookie[]>(5);
       TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr<T[]>::operator[](index): index out of range");
     }
+#if TEST_STD_VER >= 20
+    {
+      std::unique_ptr<WithCookie[]> ptr = std::make_unique_for_overwrite<WithCookie[]>(5);
+      TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = WithCookie(), "unique_ptr<T[]>::operator[](index): index out of range");
+    }
+#endif
   }
 
   // For types that don't have an array cookie, things are a bit more complicated. We can detect OOB accesses
@@ -97,14 +90,9 @@ void test() {
 #endif
 
   // Make sure that we carry the bounds information properly through conversions, assignments, etc.
-  // These tests are mostly relevant when the ABI setting is enabled (with a stateful bounds-checker),
-  // but we still run them for types with an array cookie either way.
+  // These tests are only relevant when the ABI setting is enabled (with a stateful bounds-checker).
 #if defined(_LIBCPP_ABI_BOUNDED_UNIQUE_PTR)
-  using Types = types::type_list<NoCookie, WithCookie>;
-#else
-  using Types = types::type_list<WithCookie>;
-#endif
-  types::for_each(Types(), []<class T> {
+  types::for_each(types::type_list<NoCookie, WithCookie>(), []<class T> {
     // Bounds carried through move construction
     {
       std::unique_ptr<T[]> ptr = std::make_unique<T[]>(5);
@@ -135,6 +123,7 @@ void test() {
       TEST_LIBCPP_ASSERT_FAILURE(other[6], "unique_ptr<T[]>::operator[](index): index out of range");
     }
   });
+#endif
 }
 
 template <std::size_t Size>

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp

@@ -46,6 +46,11 @@ struct WithNonTrivialDtor {
 template <class T>
 struct CustomDeleter : std::default_delete<T> {};
 
+struct NoopDeleter {
+  template <class T>
+  TEST_CONSTEXPR_CXX23 void operator()(T*) const {}
+};
+
 TEST_CONSTEXPR_CXX23 bool test() {
   // Basic test
   {
@@ -112,12 +117,33 @@ TEST_CONSTEXPR_CXX23 bool test() {
         WithNonTrivialDtor<16>,
         WithNonTrivialDtor<256>>;
     types::for_each(TrickyCookieTypes(), []<class T> {
-      types::for_each(types::type_list<std::default_delete<T[]>, CustomDeleter<T[]>>(), []<class Deleter> {
-        std::unique_ptr<T[], Deleter> p(new T[3]);
+      // Array allocated with `new T[n]`, default deleter
+      {
+        std::unique_ptr<T[], std::default_delete<T[]>> p(new T[3]);
+        assert(p[0] == T());
+        assert(p[1] == T());
+        assert(p[2] == T());
+      }
+
+      // Array allocated with `new T[n]`, custom deleter
+      {
+        std::unique_ptr<T[], CustomDeleter<T[]>> p(new T[3]);
+        assert(p[0] == T());
+        assert(p[1] == T());
+        assert(p[2] == T());
+      }
+
+      // Array not allocated with `new T[n]`, custom deleter
+      //
+      // This test aims to ensure that the implementation doesn't try to use an array cookie
+      // when there is none.
+      {
+        T array[50] = {};
+        std::unique_ptr<T[], NoopDeleter> p(&array[0]);
         assert(p[0] == T());
         assert(p[1] == T());
         assert(p[2] == T());
-      });
+      }
     });
   }
 #endif // C++20