[libcxx-commits] [libcxx] 848b20d - [libc++] Correctly handle custom deleters in hardened unique_ptr<T[]> (#110685)
via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Oct 3 08:46:02 PDT 2024
Author: Louis Dionne
Date: 2024-10-03T11:45:58-04:00
New Revision: 848b20de76def980d02bb17bcb0bdc95381876df
URL: https://github.com/llvm/llvm-project/commit/848b20de76def980d02bb17bcb0bdc95381876df
DIFF: https://github.com/llvm/llvm-project/commit/848b20de76def980d02bb17bcb0bdc95381876df.diff
LOG: [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
Added:
Modified:
libcxx/include/__memory/array_cookie.h
libcxx/include/__memory/unique_ptr.h
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/include/__memory/array_cookie.h b/libcxx/include/__memory/array_cookie.h
index 34eec643206103..10b29c9dcc78e3 100644
--- a/libcxx/include/__memory/array_cookie.h
+++ b/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
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 11215dc111e36a..6e42ef1eaa1a3c 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/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];
}
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp
index 1eaf2d5900356b..bb4ac981600f9e 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp
+++ b/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>
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp
index ebfad8ec724e51..477667274f6ece 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp
+++ b/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
More information about the libcxx-commits
mailing list