[libc++] Add an ABI setting to harden unique_ptr<T[]>::operator[] #91798
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesFull diff: https://github.com/llvm/llvm-project/pull/91798.diff 5 Files Affected:
diff --git a/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake b/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
index 4860b590dcde9..e9c90cc6bedf8 100644
--- a/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
+++ b/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
@@ -1,2 +1,2 @@
set(LIBCXX_HARDENING_MODE "fast" CACHE STRING "")
-set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS" CACHE STRING "")
+set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR" CACHE STRING "")
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 104a244cc82cc..26b3a1e4c1115 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -206,6 +206,9 @@
// - `array`.
// #define _LIBCPP_ABI_BOUNDED_ITERATORS
+// Tracks the bounds of the array owned by std::unique_ptr<T[]>, allowing it to trap when accessed out-of-bounds.
+// #define _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
+
// } ABI
// HARDENING {
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 46d9405e31596..2ad28e25a7af7 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -39,6 +39,7 @@
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
+#include <__utility/private_constructor_tag.h>
#include <cstddef>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -301,6 +302,21 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
private:
__compressed_pair<pointer, deleter_type> __ptr_;
+#ifdef _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
+ // Under the "bounded unique_ptr" ABI, we store the size of the allocation when it is known.
+ // The size of the allocation can be known when unique_ptr is created via make_unique or a
+ // similar API, however it can't be known when constructed with e.g.
+ //
+ // unique_ptr<T[]> ptr(new T[3]);
+ //
+ // In that case, we don't know the size of the allocation from within the unique_ptr.
+ // Semantically, we'd need to store `optional<size_t>`. However, since that is really
+ // heavy weight, we instead store a size_t and use 0 as a magic value meaning that we
+ // don't know the size. This means that we can't catch OOB accesses inside a unique_ptr
+ // with a 0-sized allocation, however since this is a degenerate case, it doesn't matter
+ // in practice.
+ size_t __size_ = 0;
+#endif
template <class _From>
struct _CheckArrayPointerConversion : is_same<_From, pointer> {};
@@ -397,6 +413,18 @@ 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(), std::forward<deleter_type>(__u.get_deleter())) {}
+ // Constructor used by make_unique & friends to pass the size that was allocated
+ template <class _Ptr>
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 explicit unique_ptr(
+ __private_constructor_tag, _Ptr __ptr, size_t __size) _NOEXCEPT
+ : __ptr_(__ptr, __value_init_tag())
+#ifdef _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
+ ,
+ __size_(__size)
+#endif
+ {
+ }
+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(unique_ptr&& __u) _NOEXCEPT {
reset(__u.release());
__ptr_.second() = std::forward<deleter_type>(__u.get_deleter());
@@ -434,6 +462,10 @@ 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 {
+#ifdef _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __size_ == 0 || __i < __size_, "unique_ptr<T[]>::operator[](index): index out of range");
+#endif
return __ptr_.first()[__i];
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer get() const _NOEXCEPT { return __ptr_.first(); }
@@ -624,7 +656,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>
@@ -643,7 +675,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>
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
new file mode 100644
index 0000000000000..dd722f0c838ef
--- /dev/null
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: c++03, c++11
+// UNSUPPORTED: libcpp-hardening-mode=none
+// XFAIL: libcpp-hardening-mode=debug && availability-verbose_abort-missing
+// REQUIRES: libcpp-has-abi-bounded-unique_ptr
+
+// <memory>
+//
+// unique_ptr<T[]>
+//
+// T& operator[](std::size_t);
+
+// This test ensures that we catch an out-of-bounds access in std::unique_ptr<T[]>::operator[]
+// when unique_ptr has the appropriate ABI configuration.
+
+#include <memory>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+ {
+ std::unique_ptr<int[]> ptr = std::make_unique<int[]>(5);
+ TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = 42, "unique_ptr<T[]>::operator[](index): index out of range");
+ }
+
+#if TEST_STD_VER >= 20
+ {
+ std::unique_ptr<int[]> ptr = std::make_unique_for_overwrite<int[]>(5);
+ TEST_LIBCPP_ASSERT_FAILURE(ptr[6] = 42, "unique_ptr<T[]>::operator[](index): index out of range");
+ }
+#endif
+
+ return 0;
+}
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index c81b56b1af547..f154840430ec5 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -312,6 +312,7 @@ def _getAndroidDeviceApi(cfg):
"_LIBCPP_NO_VCRUNTIME": "libcpp-no-vcruntime",
"_LIBCPP_ABI_VERSION": "libcpp-abi-version",
"_LIBCPP_ABI_BOUNDED_ITERATORS": "libcpp-has-abi-bounded-iterators",
+ "_LIBCPP_ABI_BOUNDED_UNIQUE_PTR": "libcpp-has-abi-bounded-unique_ptr",
"_LIBCPP_HAS_NO_FILESYSTEM": "no-filesystem",
"_LIBCPP_HAS_NO_RANDOM_DEVICE": "no-random-device",
"_LIBCPP_HAS_NO_LOCALIZATION": "no-localization",
|
|
Note that this patch is kind of a WIP. Another approach could be to query the compiler or the system allocator for the size of the allocation. I actually think the compiler knows the size of the allocation cause it encodes it at the front of the allocation when calling |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Only when the element type has a non-trivial destructor. |
|
A couple of things I would like us to investigate:
|
That sounds like for such types we could use it for runtime check in EDIT: How crazy would it be to allow this for all types? |
|
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies describes array cookies. If the cookie is present, I can't think of any reason it would be a problem for libc++ to read it. If the cookie isn't present, though, we can't access it, and forcing the compiler to generate a cookie where it normally wouldn't generate one is ABI break. |
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
4841e1a to
253485d
Compare
|
I updated the patch with a WIP attempt to use the array cookie when one is present. There are still things to fix and comments to address (e.g. maybe using |
|
@vitalybuka I just stumbled upon |
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
253485d to
4a453cf
Compare
| // 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> > {}; |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
This seems reasonable for now.
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
4a453cf to
0f56d47
Compare
I see this feature for the first time :) Looks like @filcab added the feature. Before that custom new was omitted, then And the doc string was gone with https://reviews.llvm.org/D133349 I believe this is off by default "use at your own risk" feature. I guess it should be easy to support this case by |
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
3aae45d to
ffd681f
Compare
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
ffd681f to
c37a05e
Compare
This patch adds an ABI configuration that allows bounds-checking in unique_ptr<T[]>::operator[] when it has been constructed with bounds information in the API. The patch also adds support for bounds-checking when an array cookie is known to exist, which allows validating bounds without even changing the ABI. Drive-by changes: - Improve the tests for `operator[]` - Improve the tests for `.get()` - Add a test for incomplete types support
ldionne
force-pushed
the
review/harden-unique_ptr-array
branch
from
b23d82d to
76fc808
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/2730 Here is the relevant piece of the build log for the reference |
I looked into it and I don't think this is caused by the patch, since the test doesn't use |
…vm#91798) This allows catching OOB accesses inside `unique_ptr<T[]>` when the size of the allocation is known. The size of the allocation can be known when the unique_ptr has been created with make_unique & friends or when the type necessitates an array cookie before the allocation.
…or[] (llvm#91798)" This reverts commit 45a09d1.
…1798) This allows catching OOB accesses inside `unique_ptr<T[]>` when the size of the allocation is known. The size of the allocation can be known when the unique_ptr has been created with make_unique & friends or when the type necessitates an array cookie before the allocation. This is a re-aplpication of 45a09d1 which had been reverted in f11abac due to unrelated CI failures.
|
This makes the attached program crash (…in most runs, not 100% of the time): lldb shows that this indeed this check: The program uses a unique_ptr with a custom deleter to refer to raw malloc-ed memory, "to allow having uninitialized entries inside it". It has this typedef: It uses this typedef with an array type: It creates objects of this unique_ptr like so:
The custom deleter looks like so ( The class containing Is this a valid use of unique_ptr<T[]>? If so, should this hardening only be used for unique_ptrs that don't have a custom deleter? |
|
Looking through |
|
I think you're entirely correct. I'm working on a patch. |
|
In fact, the memory held by the |
|
What about a case when a If we skip the question why would one do that at all (I'm about to ask the author(s)), it seems like this is a valid use of the It seems like the newly added bounds check accesses invalid memory trying to read the array cookie, which is not there. In the above compiler explorer link one can see how it results in a memory sanitizer error. Internally we observe a similar code triggering an "index out of range" assertion when accessing a valid index. |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/incomplete.sh.cpp
Show resolved
Hide resolved
...tilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/assert.subscript.pass.cpp
Show resolved
Hide resolved
| // 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) {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
SIZE_MAX is supposed to be defined in <cstdint>, can you check why your platform doesn't provide it?
There was a problem hiding this comment.
It is provided by <cstdint> just fine. The user code essentially reduces down to this:
#include <limits.h>
#include <stdint.h>
#undef SIZE_MAX
#include <string>Which makes it more clear that this is the fault of user code.
That said, I guess it wouldn't hurt if libcxx would use __SIZE_MAX__ either, if that's a kind define that libcxx can rely on being available (I guess both GCC and Clang provide such defines), which would be more resilient to user macro redefinitions. (I'm not aware of the original reason for why the user code wants to do #undef SIZE_MAX.)
|
@var-const Thanks for the comments! See #111704. |
I'm not certain, but I think it's invalid. I think we're walking on an extremely thin line here, see below. First, about whether this should be valid (I don't think you disagree, I just want to write it down): Now, about whether this is actually, technically, valid. unique.ptr#single.general-1 says this:
I am tempted to say that the requirements on a custom deleter's validity also apply to the default deleter. Furthermore, that may be stretching it a bit, but I would say that this requirement is not satisfied in the case you showed above because the expression To be unambiguous, we should perhaps add something to the various Frankly, I'm not certain what to do with this. I feel like it would be really unfortunate to drop this hardening check on a slightly ambiguous technicality since it can deliver a ton of value in the real world. I think I'll email the reflector to see what the sentiment is like over there. |
|
Thanks for the detailed analysis. I totally agree with the conclusions here. The value of this hardening is obvious, while the ability to (mis-)use unique_ptr this way is questionable on its own. Luckily (though no surprise ;) this was an isolated case and the code has already been fixed.
Making this part of the standard unambiguous would definitely be welcome.
I almost feel sorry for bringing this up ;) But hopefully, this will result in improving the standard rather than creating an obstacle for library hardenings. |
undef SIZE_MAX caused building aria2 with libc++20 to fail, because libc++ uses this definition internally. ref: mstorsjo/llvm-mingw#460 llvm/llvm-project#91798 (comment)
This allows catching OOB accesses inside
unique_ptr<T[]>when the sizeof the allocation is known. The size of the allocation can be known when
the unique_ptr has been created with make_unique & friends or when the
type necessitates an array cookie before the allocation.