[libc-commits] [libc] [llvm] [libc] Add hardening for FixedVector data structure and fix exposed bug. (PR #122159)
Alexey Samsonov via libc-commits
libc-commits at lists.llvm.org
Wed Jan 8 12:34:28 PST 2025
https://github.com/vonosmas updated https://github.com/llvm/llvm-project/pull/122159
>From e924f71eabffe3e1aa2628e24fc29725bf1a149e Mon Sep 17 00:00:00 2001
From: Alexey Samsonov <samsonov at google.com>
Date: Wed, 8 Jan 2025 11:29:38 -0800
Subject: [PATCH 1/3] [libc] Add hardening for FixedVector data structure and
fix exposed bug.
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.
---
libc/src/__support/CMakeLists.txt | 2 +
libc/src/__support/fixedvector.h | 53 ++++++++++++-------
libc/src/stdlib/exit_handler.h | 2 +-
.../llvm-project-overlay/libc/BUILD.bazel | 2 +
4 files changed, 40 insertions(+), 19 deletions(-)
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",
],
)
>From 18db344b9f60889409bb55aaf820e8c82e18d283 Mon Sep 17 00:00:00 2001
From: Alexey Samsonov <samsonov at google.com>
Date: Wed, 8 Jan 2025 12:26:32 -0800
Subject: [PATCH 2/3] Fix formatting
---
libc/src/__support/fixedvector.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libc/src/__support/fixedvector.h b/libc/src/__support/fixedvector.h
index 9671e708a938b8..258afaaa3d5c0b 100644
--- a/libc/src/__support/fixedvector.h
+++ b/libc/src/__support/fixedvector.h
@@ -27,7 +27,8 @@ template <typename T, size_t CAPACITY> class FixedVector {
LIBC_INLINE constexpr FixedVector() = default;
using iterator = typename cpp::array<T, CAPACITY>::iterator;
- LIBC_INLINE constexpr FixedVector(iterator begin, iterator end) : store{}, item_count{} {
+ LIBC_INLINE constexpr FixedVector(iterator begin, iterator end)
+ : store{}, item_count{} {
for (; begin != end; ++begin)
LIBC_ASSERT(push_back(*begin));
}
@@ -39,7 +40,8 @@ template <typename T, size_t CAPACITY> class FixedVector {
LIBC_ASSERT(push_back(*begin));
}
- LIBC_INLINE 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)
LIBC_ASSERT(push_back(value));
}
@@ -98,7 +100,9 @@ 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.
- LIBC_INLINE 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() {
>From 96fd86841fa5b89a5dfeb415327097c57f4108fb Mon Sep 17 00:00:00 2001
From: Alexey Samsonov <samsonov at google.com>
Date: Wed, 8 Jan 2025 12:31:57 -0800
Subject: [PATCH 3/3] Move business logic outside of LIBC_ASSERT, so that it's
not optimized away.
---
libc/src/__support/fixedvector.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/libc/src/__support/fixedvector.h b/libc/src/__support/fixedvector.h
index 258afaaa3d5c0b..34601f86dc0176 100644
--- a/libc/src/__support/fixedvector.h
+++ b/libc/src/__support/fixedvector.h
@@ -29,21 +29,24 @@ template <typename T, size_t CAPACITY> class FixedVector {
using iterator = typename cpp::array<T, CAPACITY>::iterator;
LIBC_INLINE constexpr FixedVector(iterator begin, iterator end)
: store{}, item_count{} {
+ LIBC_ASSERT(begin + CAPACITY >= end);
for (; begin != end; ++begin)
- LIBC_ASSERT(push_back(*begin));
+ push_back(*begin);
}
using const_iterator = typename cpp::array<T, CAPACITY>::const_iterator;
LIBC_INLINE constexpr FixedVector(const_iterator begin, const_iterator end)
: store{}, item_count{} {
+ LIBC_ASSERT(begin + CAPACITY >= end);
for (; begin != end; ++begin)
- LIBC_ASSERT(push_back(*begin));
+ push_back(*begin);
}
LIBC_INLINE constexpr FixedVector(size_t count, const T &value)
: store{}, item_count{} {
+ LIBC_ASSERT(count <= CAPACITY);
for (size_t i = 0; i < count; ++i)
- LIBC_ASSERT(push_back(value));
+ push_back(value);
}
LIBC_INLINE constexpr bool push_back(const T &obj) {
More information about the libc-commits
mailing list