From e900a63491d91639e8df3b229c7ca2478eae8727 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Tue, 1 Oct 2024 11:04:30 -0400 Subject: [PATCH 1/3] [libc++] Correctly handle custom deleters in hardened unique_ptr 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 --- libcxx/include/__memory/array_cookie.h | 2 +- libcxx/include/__memory/unique_ptr.h | 35 ++++++++++++++----- .../assert.subscript.pass.cpp | 31 +++++++--------- .../op_subscript.runtime.pass.cpp | 32 +++++++++++++++-- 4 files changed, 69 insertions(+), 31 deletions(-) diff --git a/libcxx/include/__memory/array_cookie.h b/libcxx/include/__memory/array_cookie.h index 34eec6432061036..10b29c9dcc78e34 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 11215dc111e36a8..6e42ef1eaa1a3cd 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 +struct __is_default_deleter : false_type {}; + +template +struct __is_default_deleter > : true_type {}; + template 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`, 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 ::value, int> = 0> + template ::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 ::value, int> = 0> + template ::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 ::value, int> = 0> + template ::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 ::value, int> = 0> + template ::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(std::__to_address(__ptr_), __i), "unique_ptr::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 1eaf2d5900356be..66a7882c0ad34ed 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 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 ptr(new WithCookie[5]); - TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr::operator[](index): index out of range"); - } - { - std::unique_ptr ptr = std::make_unique(5); - TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr::operator[](index): index out of range"); - } -#if TEST_STD_VER >= 20 - { - std::unique_ptr ptr = std::make_unique_for_overwrite(5); - TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = WithCookie(), "unique_ptr::operator[](index): index out of range"); - } -#endif + std::unique_ptr ptr(new WithCookie[5]); + TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr::operator[](index): index out of range"); } - - // Check with a custom deleter { - std::unique_ptr ptr(new WithCookie[5]); + std::unique_ptr ptr = std::make_unique(5); TEST_LIBCPP_ASSERT_FAILURE(ptr[6], "unique_ptr::operator[](index): index out of range"); } +#if TEST_STD_VER >= 20 + { + std::unique_ptr ptr = std::make_unique_for_overwrite(5); + TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = WithCookie(), "unique_ptr::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 ebfad8ec724e517..ab33d38cd15a6e8 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 struct CustomDeleter : std::default_delete {}; +struct NoopDeleter { + template + 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(), [] { - types::for_each(types::type_list, CustomDeleter>(), [] { - std::unique_ptr p(new T[3]); + // Array allocated with `new T[n]`, default deleter + { + std::unique_ptr> 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> 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 p(&array[0]); assert(p[0] == T()); assert(p[1] == T()); assert(p[2] == T()); - }); + } }); } #endif // C++20 From f10487ce5980105c747ffc998b3e0860e9fa7a83 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Thu, 3 Oct 2024 08:35:30 -0400 Subject: [PATCH 2/3] Fix constexpr --- .../unique.ptr.observers/op_subscript.runtime.pass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ab33d38cd15a6e8..477667274f6ecef 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 @@ -48,7 +48,7 @@ struct CustomDeleter : std::default_delete {}; struct NoopDeleter { template - TEST_CONSTEXPR void operator()(T*) const {} + TEST_CONSTEXPR_CXX23 void operator()(T*) const {} }; TEST_CONSTEXPR_CXX23 bool test() { From 00729e32ef180683ad93a85a1920e38aff87f25c Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Thu, 3 Oct 2024 08:41:00 -0400 Subject: [PATCH 3/3] Fix test that was checking incorrect stuff --- .../unique.ptr.observers/assert.subscript.pass.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) 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 66a7882c0ad34ed..bb4ac981600f9ee 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 @@ -90,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; -#else - using Types = types::type_list; -#endif - types::for_each(Types(), [] { + types::for_each(types::type_list(), [] { // Bounds carried through move construction { std::unique_ptr ptr = std::make_unique(5); @@ -128,6 +123,7 @@ void test() { TEST_LIBCPP_ASSERT_FAILURE(other[6], "unique_ptr::operator[](index): index out of range"); } }); +#endif } template