-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc] Add hardening for FixedVector data structure and fix exposed bug. #122159
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
Conversation
Add LIBC_ASSERT statements to FixedVector implementation, and zero out the memory when the elements are removed to flag out-of-bound access and dangling pointer/reference access. This change unmasks the bug in one of FixedVector uses for atexit handlers: dangling reference use, which was actually led to crashes in the wild (with prod blockstore implementation). Fix it in this CL.
|
@llvm/pr-subscribers-libc Author: Alexey Samsonov (vonosmas) ChangesAdd LIBC_ASSERT statements to FixedVector implementation, and zero out the memory when the elements are removed to flag out-of-bound access and dangling pointer/reference access. This change unmasks the bug in one of FixedVector uses for atexit handlers: dangling reference use, which was actually led to crashes in the wild (with prod blockstore implementation). Fix it in this CL. Full diff: https://github.com/llvm/llvm-project/pull/122159.diff 4 Files Affected:
diff --git a/libc/src/__support/CMakeLists.txt b/libc/src/__support/CMakeLists.txt
index 4e90aad9a45b40..5090dc218cda4a 100644
--- a/libc/src/__support/CMakeLists.txt
+++ b/libc/src/__support/CMakeLists.txt
@@ -267,7 +267,9 @@ add_header_library(
HDRS
fixedvector.h
DEPENDS
+ .libc_assert
libc.src.__support.CPP.array
+ libc.src.string.memory_utils.inline_memset
)
add_header_library(
diff --git a/libc/src/__support/fixedvector.h b/libc/src/__support/fixedvector.h
index 7ac0c230f9c536..9671e708a938b8 100644
--- a/libc/src/__support/fixedvector.h
+++ b/libc/src/__support/fixedvector.h
@@ -10,9 +10,10 @@
#define LLVM_LIBC_SRC___SUPPORT_FIXEDVECTOR_H
#include "src/__support/CPP/array.h"
-
#include "src/__support/CPP/iterator.h"
+#include "src/__support/libc_assert.h"
#include "src/__support/macros/config.h"
+#include "src/string/memory_utils/inline_memset.h"
namespace LIBC_NAMESPACE_DECL {
@@ -23,27 +24,27 @@ template <typename T, size_t CAPACITY> class FixedVector {
size_t item_count = 0;
public:
- constexpr FixedVector() = default;
+ LIBC_INLINE constexpr FixedVector() = default;
using iterator = typename cpp::array<T, CAPACITY>::iterator;
- constexpr FixedVector(iterator begin, iterator end) : store{}, item_count{} {
+ LIBC_INLINE constexpr FixedVector(iterator begin, iterator end) : store{}, item_count{} {
for (; begin != end; ++begin)
- push_back(*begin);
+ LIBC_ASSERT(push_back(*begin));
}
using const_iterator = typename cpp::array<T, CAPACITY>::const_iterator;
- constexpr FixedVector(const_iterator begin, const_iterator end)
+ LIBC_INLINE constexpr FixedVector(const_iterator begin, const_iterator end)
: store{}, item_count{} {
for (; begin != end; ++begin)
- push_back(*begin);
+ LIBC_ASSERT(push_back(*begin));
}
- constexpr FixedVector(size_t count, const T &value) : store{}, item_count{} {
+ LIBC_INLINE constexpr FixedVector(size_t count, const T &value) : store{}, item_count{} {
for (size_t i = 0; i < count; ++i)
- push_back(value);
+ LIBC_ASSERT(push_back(value));
}
- constexpr bool push_back(const T &obj) {
+ LIBC_INLINE constexpr bool push_back(const T &obj) {
if (item_count == CAPACITY)
return false;
store[item_count] = obj;
@@ -51,27 +52,43 @@ template <typename T, size_t CAPACITY> class FixedVector {
return true;
}
- constexpr const T &back() const { return store[item_count - 1]; }
+ LIBC_INLINE constexpr const T &back() const {
+ LIBC_ASSERT(!empty());
+ return store[item_count - 1];
+ }
- constexpr T &back() { return store[item_count - 1]; }
+ LIBC_INLINE constexpr T &back() {
+ LIBC_ASSERT(!empty());
+ return store[item_count - 1];
+ }
- constexpr bool pop_back() {
+ LIBC_INLINE constexpr bool pop_back() {
if (item_count == 0)
return false;
+ inline_memset(&store[item_count - 1], 0, sizeof(T));
--item_count;
return true;
}
- constexpr T &operator[](size_t idx) { return store[idx]; }
+ LIBC_INLINE constexpr T &operator[](size_t idx) {
+ LIBC_ASSERT(idx < item_count);
+ return store[idx];
+ }
- constexpr const T &operator[](size_t idx) const { return store[idx]; }
+ LIBC_INLINE constexpr const T &operator[](size_t idx) const {
+ LIBC_ASSERT(idx < item_count);
+ return store[idx];
+ }
- constexpr bool empty() const { return item_count == 0; }
+ LIBC_INLINE constexpr bool empty() const { return item_count == 0; }
- constexpr size_t size() const { return item_count; }
+ LIBC_INLINE constexpr size_t size() const { return item_count; }
// Empties the store for all practical purposes.
- constexpr void reset() { item_count = 0; }
+ LIBC_INLINE constexpr void reset() {
+ inline_memset(store.data(), 0, sizeof(T) * item_count);
+ item_count = 0;
+ }
// This static method does not free up the resources held by |store|,
// say by calling `free` or something similar. It just does the equivalent
@@ -81,7 +98,7 @@ template <typename T, size_t CAPACITY> class FixedVector {
// dynamically allocated storate. So, the `destroy` method like this
// matches the `destroy` API of those other data structures so that users
// can easily swap one data structure for the other.
- static void destroy(FixedVector<T, CAPACITY> *store) { store->reset(); }
+ LIBC_INLINE static void destroy(FixedVector<T, CAPACITY> *store) { store->reset(); }
using reverse_iterator = typename cpp::array<T, CAPACITY>::reverse_iterator;
LIBC_INLINE constexpr reverse_iterator rbegin() {
diff --git a/libc/src/stdlib/exit_handler.h b/libc/src/stdlib/exit_handler.h
index 9720c5473940ee..e9d163dfe90244 100644
--- a/libc/src/stdlib/exit_handler.h
+++ b/libc/src/stdlib/exit_handler.h
@@ -48,7 +48,7 @@ LIBC_INLINE void stdc_at_exit_func(void *payload) {
LIBC_INLINE void call_exit_callbacks(ExitCallbackList &callbacks) {
handler_list_mtx.lock();
while (!callbacks.empty()) {
- AtExitUnit &unit = callbacks.back();
+ AtExitUnit unit = callbacks.back();
callbacks.pop_back();
handler_list_mtx.unlock();
unit.callback(unit.payload);
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index 09811eb06ff02f..ac3f5034d2bfa4 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@@ -670,6 +670,8 @@ libc_support_library(
deps = [
":__support_cpp_array",
":__support_cpp_iterator",
+ ":__support_libc_assert",
+ ":string_memory_utils",
],
)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
michaelrj-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add LIBC_ASSERT statements to FixedVector implementation, and zero out the memory when the elements are removed to flag out-of-bound access and dangling pointer/reference access.
This change unmasks the bug in one of FixedVector uses for atexit handlers: dangling reference use, which was actually led to crashes in the wild (with prod blockstore implementation). Fix it in this CL.