[libc++] Correctly handle custom deleters in hardened unique_ptr<T[]> #110685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 llvm#110683
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesIt 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 Fixes #110683 Full diff: https://github.com/llvm/llvm-project/pull/110685.diff 4 Files Affected:
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..66a7882c0ad34e 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
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..ab33d38cd15a6e 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 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
|
ldionne
commented
Oct 1, 2024
| // This test aims to ensure that the implementation doesn't try to use an array cookie | ||
| // when there is none. | ||
| { | ||
| T array[50] = {}; |
There was a problem hiding this comment.
@nico I verified locally and this test breaks down without my patch. That's a smaller version of your reproducer.
nico
approved these changes
Oct 1, 2024
I am iffy about exposing a vendor extension for this. Instead I think it would make more sense to pursue standardization of |
How does would that API help? Would it call (To be clear I agree about the iffy and don't want to argue for anything here, just asking for my understanding.) |
We know the size of the allocation because it gets passed to the function. The same would be true with a custom deleter, e.g. If that were the case, we could stash the size passed to |
|
Looks like the two failing bots are true failures: stage2 (generic-cxx11, clang-19, clang++-19): macos (apple-system-hardened, macos-13): |
|
Sorry for the delay, I thought I had merged this already. The tests should be good this time around. |
xgupta
pushed a commit
to xgupta/llvm-project
that referenced
this pull request
Oct 4, 2024
…llvm#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 llvm#110683
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_uniquethat 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