Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Add an ABI setting to harden unique_ptr<T[]>::operator[] #91798

Merged
merged 1 commit into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
set(LIBCXX_HARDENING_MODE "fast" CACHE STRING "")
set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR" CACHE STRING "")
set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR" CACHE STRING "")
1 change: 1 addition & 0 deletions libcxx/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ set(files
__memory/allocator_arg_t.h
__memory/allocator_destructor.h
__memory/allocator_traits.h
__memory/array_cookie.h
__memory/assume_aligned.h
__memory/auto_ptr.h
__memory/builtin_new_allocator.h
Expand Down
7 changes: 7 additions & 0 deletions libcxx/include/__configuration/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@
# define _LIBCPP_ABI_NO_COMPRESSED_PAIR_PADDING
#endif

// Tracks the bounds of the array owned by std::unique_ptr<T[]>, allowing it to trap when accessed out-of-bounds.
// Note that limited bounds checking is also available outside of this ABI configuration, but only some categories
// of types can be checked.
//
// ABI impact: This causes the layout of std::unique_ptr<T[]> to change and its size to increase.
ldionne marked this conversation as resolved.
Show resolved Hide resolved
// #define _LIBCPP_ABI_BOUNDED_UNIQUE_PTR

#if defined(_LIBCPP_COMPILER_CLANG_BASED)
# if defined(__APPLE__)
# if defined(__i386__) || defined(__x86_64__)
Expand Down
55 changes: 55 additions & 0 deletions libcxx/include/__memory/array_cookie.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP___MEMORY_ARRAY_COOKIE_H
#define _LIBCPP___MEMORY_ARRAY_COOKIE_H

#include <__config>
#include <__configuration/abi.h>
#include <__type_traits/integral_constant.h>
#include <__type_traits/is_trivially_destructible.h>
#include <__type_traits/negation.h>
#include <cstddef>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

_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`.
//
// 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
// than the Itanium ABI, we assume there are no array cookies.
//
// [1]: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies
#ifdef _LIBCPP_ABI_ITANIUM
// TODO: Use a builtin instead
// TODO: We should factor in the choice of the usual deallocation function in this determination.
template <class _Tp>
struct __has_array_cookie : _Not<is_trivially_destructible<_Tp> > {};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efriedma-quic What do you think about this? It is a bit more conservative than we could be, but I don't really want to start trying to implement the resolution of operator delete[] in the library. It looks like a compiler builtin should be fairly easy to provide for both getting the array cookie and telling us whether an array cookie is available.

I'm curious to know whether you think this is a reasonable first implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable for now.

#else
template <class _Tp>
struct __has_array_cookie : false_type {};
#endif

template <class _Tp>
// Avoid failures when -fsanitize-address-poison-custom-array-cookie is enabled
_LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_SANITIZE("address") size_t __get_array_cookie(_Tp const* __ptr) {
static_assert(
__has_array_cookie<_Tp>::value, "Trying to access the array cookie of a type that is not guaranteed to have one");
size_t const* __cookie = reinterpret_cast<size_t const*>(__ptr) - 1; // TODO: Use a builtin instead
return *__cookie;
}

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___MEMORY_ARRAY_COOKIE_H
128 changes: 123 additions & 5 deletions libcxx/include/__memory/unique_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@
#ifndef _LIBCPP___MEMORY_UNIQUE_PTR_H
#define _LIBCPP___MEMORY_UNIQUE_PTR_H

#include <__assert>
#include <__compare/compare_three_way.h>
#include <__compare/compare_three_way_result.h>
#include <__compare/three_way_comparable.h>
#include <__config>
#include <__functional/hash.h>
#include <__functional/operations.h>
#include <__memory/allocator_traits.h> // __pointer
#include <__memory/array_cookie.h>
#include <__memory/auto_ptr.h>
#include <__memory/compressed_pair.h>
#include <__memory/pointer_traits.h>
#include <__type_traits/add_lvalue_reference.h>
#include <__type_traits/common_type.h>
#include <__type_traits/conditional.h>
Expand All @@ -27,6 +30,7 @@
#include <__type_traits/integral_constant.h>
#include <__type_traits/is_array.h>
#include <__type_traits/is_assignable.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_constructible.h>
#include <__type_traits/is_convertible.h>
#include <__type_traits/is_function.h>
Expand All @@ -41,7 +45,9 @@
#include <__utility/declval.h>
#include <__utility/forward.h>
#include <__utility/move.h>
#include <__utility/private_constructor_tag.h>
#include <cstddef>
#include <cstdint>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand Down Expand Up @@ -292,6 +298,91 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
}
};

// Bounds checking in unique_ptr<T[]>
ldionne marked this conversation as resolved.
Show resolved Hide resolved
// ==================================
//
// We provide some helper classes that allow bounds checking when accessing a unique_ptr<T[]>.
// There are a few cases where bounds checking can be implemented:
//
// 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`.
// 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.
//
// 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
// an ABI break from the default configuration.
//
// Note that even under this ABI configuration, we can't always know the size of the unique_ptr.
// Indeed, the size of the allocation can only be known when the unique_ptr is created via
// make_unique or a similar API. For example, it can't be known when constructed from an arbitrary
// pointer, in which case we are not able to check the bounds on access:
//
// unique_ptr<T[], MyDeleter> ptr(new T[3]);
//
// When we don't know the size of the allocation via the API used to create the unique_ptr, we
// 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
// 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.
//
// (1) Implementation where we rely on the array cookie to know the size of the allocation, if
// an array cookie exists.
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>
_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.
if (__libcpp_is_constant_evaluated())
return true;
size_t __cookie = std::__get_array_cookie(__ptr);
return __index < __cookie;
}

template <class _Tp, __enable_if_t<!__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
}
};

// (2) Implementation where we store the size in the class whenever we have it.
//
// Semantically, we'd need to store the size as an optional<size_t>. However, since that
// is really heavy weight, we instead store a size_t and use SIZE_MAX as a magic value
// meaning that we don't know the size.
struct __unique_ptr_array_bounds_stored {
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __unique_ptr_array_bounds_stored() : __size_(SIZE_MAX) {}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __unique_ptr_array_bounds_stored(size_t __size) : __size_(__size) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In file included from SSHSession.cc:35:
In file included from ./SSHSession.h:39:
In file included from ./a2netcompat.h:94:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/string:648:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/string_view:958:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/algorithm:1851:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/__algorithm/for_each.h:16:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/__ranges/movable_box.h:21:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/optional:1297:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/memory:944:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/__memory/inout_ptr.h:16:
In file included from /build/install/x86_64-w64-mingw32/include/c++/v1/__memory/shared_ptr.h:32:
/build/install/x86_64-w64-mingw32/include/c++/v1/__memory/unique_ptr.h:379:88: error: use of undeclared identifier 'SIZE_MAX'
  379 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __unique_ptr_array_bounds_stored() : __size_(SIZE_MAX) {}
      |                                                                                        ^
1 error generated.

This causes the above error when building aria2. If I replace it with __SIZE_MAX__ I won't get the error.

llvm-mingw nightly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIZE_MAX is supposed to be defined in <cstdint>, can you check why your platform doesn't provide it?


// Use the array cookie if there's one
template <class _Tp, __enable_if_t<__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;
size_t __cookie = std::__get_array_cookie(__ptr);
return __index < __cookie;
}

// Otherwise, fall back on the stored size (if any)
template <class _Tp, __enable_if_t<!__has_array_cookie<_Tp>::value, int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Tp*, size_t __index) const {
return __index < __size_;
}

private:
size_t __size_;
};

template <class _Tp, class _Dp>
class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp> {
public:
Expand All @@ -300,8 +391,9 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
typedef typename __pointer<_Tp, deleter_type>::type pointer;

// A unique_ptr contains the following members which may be trivially relocatable:
// - pointer : this may be trivially relocatable, so it's checked
// - pointer: this may be trivially relocatable, so it's checked
// - deleter_type: this may be trivially relocatable, so it's checked
// - (optionally) size: this is trivially relocatable
//
// This unique_ptr implementation only contains a pointer to the unique object and a deleter, so there are no
// references to itself. This means that the entire structure is trivially relocatable if its members are.
Expand All @@ -311,7 +403,16 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
void>;

private:
template <class _Up, class _OtherDeleter>
friend class unique_ptr;

_LIBCPP_COMPRESSED_PAIR(pointer, __ptr_, deleter_type, __deleter_);
#ifdef _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
using _BoundsChecker = __unique_ptr_array_bounds_stored;
#else
using _BoundsChecker = __unique_ptr_array_bounds_stateless;
#endif
_LIBCPP_NO_UNIQUE_ADDRESS _BoundsChecker __checker_;

template <class _From>
struct _CheckArrayPointerConversion : is_same<_From, pointer> {};
Expand Down Expand Up @@ -373,6 +474,12 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
: __ptr_(__p),
__deleter_() {}

// Private constructor used by make_unique & friends to pass the size that was allocated
template <class _Tag, class _Ptr, __enable_if_t<is_same<_Tag, __private_constructor_tag>::value, int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 explicit unique_ptr(_Tag, _Ptr __ptr, size_t __size) _NOEXCEPT
: __ptr_(__ptr),
__checker_(__size) {}

template <class _Pp,
bool _Dummy = true,
class = _EnableIfDeleterConstructible<_LValRefType<_Dummy> >,
Expand Down Expand Up @@ -411,11 +518,13 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr(unique_ptr&& __u) _NOEXCEPT
: __ptr_(__u.release()),
__deleter_(std::forward<deleter_type>(__u.get_deleter())) {}
__deleter_(std::forward<deleter_type>(__u.get_deleter())),
__checker_(std::move(__u.__checker_)) {}

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(unique_ptr&& __u) _NOEXCEPT {
reset(__u.release());
__deleter_ = std::forward<deleter_type>(__u.get_deleter());
__checker_ = std::move(std::move(__u.__checker_));
ldionne marked this conversation as resolved.
Show resolved Hide resolved
return *this;
}

Expand All @@ -425,7 +534,8 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
class = _EnableIfDeleterConvertible<_Ep> >
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr(unique_ptr<_Up, _Ep>&& __u) _NOEXCEPT
: __ptr_(__u.release()),
__deleter_(std::forward<_Ep>(__u.get_deleter())) {}
__deleter_(std::forward<_Ep>(__u.get_deleter())),
__checker_(std::move(__u.__checker_)) {}

template <class _Up,
class _Ep,
Expand All @@ -434,6 +544,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(unique_ptr<_Up, _Ep>&& __u) _NOEXCEPT {
reset(__u.release());
__deleter_ = std::forward<_Ep>(__u.get_deleter());
__checker_ = std::move(__u.__checker_);
return *this;
}

Expand All @@ -451,6 +562,8 @@ 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),
"unique_ptr<T[]>::operator[](index): index out of range");
return __ptr_[__i];
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer get() const _NOEXCEPT { return __ptr_; }
Expand All @@ -467,20 +580,24 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer release() _NOEXCEPT {
pointer __t = __ptr_;
__ptr_ = pointer();
// The deleter and the optional bounds-checker are left unchanged. The bounds-checker
// will be reinitialized appropriately when/if the unique_ptr gets assigned-to or reset.
return __t;
}

template <class _Pp, __enable_if_t<_CheckArrayPointerConversion<_Pp>::value, int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 void reset(_Pp __p) _NOEXCEPT {
pointer __tmp = __ptr_;
__ptr_ = __p;
__checker_ = _BoundsChecker();
if (__tmp)
__deleter_(__tmp);
}

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 void reset(nullptr_t = nullptr) _NOEXCEPT {
pointer __tmp = __ptr_;
__ptr_ = nullptr;
__checker_ = _BoundsChecker();
if (__tmp)
__deleter_(__tmp);
}
Expand All @@ -489,6 +606,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
using std::swap;
swap(__ptr_, __u.__ptr_);
swap(__deleter_, __u.__deleter_);
swap(__checker_, __u.__checker_);
}
};

Expand Down Expand Up @@ -645,7 +763,7 @@ template <class _Tp>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 typename __unique_if<_Tp>::__unique_array_unknown_bound
make_unique(size_t __n) {
typedef __remove_extent_t<_Tp> _Up;
return unique_ptr<_Tp>(new _Up[__n]());
return unique_ptr<_Tp>(__private_constructor_tag(), new _Up[__n](), __n);
}

template <class _Tp, class... _Args>
Expand All @@ -664,7 +782,7 @@ make_unique_for_overwrite() {
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 typename __unique_if<_Tp>::__unique_array_unknown_bound
make_unique_for_overwrite(size_t __n) {
return unique_ptr<_Tp>(new __remove_extent_t<_Tp>[__n]);
return unique_ptr<_Tp>(__private_constructor_tag(), new __remove_extent_t<_Tp>[__n], __n);
}

template <class _Tp, class... _Args>
Expand Down
1 change: 1 addition & 0 deletions libcxx/include/module.modulemap
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,7 @@ module std [system] {
module allocator_destructor { header "__memory/allocator_destructor.h" }
module allocator_traits { header "__memory/allocator_traits.h" }
module assume_aligned { header "__memory/assume_aligned.h" }
module array_cookie { header "__memory/array_cookie.h" }
module auto_ptr { header "__memory/auto_ptr.h" }
module builtin_new_allocator { header "__memory/builtin_new_allocator.h" }
module compressed_pair { header "__memory/compressed_pair.h" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

// UNSUPPORTED: libcpp-has-abi-fix-unordered-container-size-type, libcpp-abi-no-compressed-pair-padding

// std::unique_ptr is used as an implementation detail of the unordered containers, so the layout of
// unordered containers changes when bounded unique_ptr is enabled.
// UNSUPPORTED: libcpp-has-abi-bounded-unique_ptr

#include <cstdint>
#include <unordered_map>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

// UNSUPPORTED: libcpp-has-abi-fix-unordered-container-size-type, libcpp-abi-no-compressed-pair-padding

// std::unique_ptr is used as an implementation detail of the unordered containers, so the layout of
// unordered containers changes when bounded unique_ptr is enabled.
// UNSUPPORTED: libcpp-has-abi-bounded-unique_ptr

#include <cstdint>
#include <unordered_set>

Expand Down
Loading
Loading