[libcxx-commits] [libcxx] [libc++][hardening] Classify assertions related to leaks and syscalls. (PR #77164)
via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 5 16:53:48 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
Author: Konstantin Varlamov (var-const)
<details>
<summary>Changes</summary>
Introduce two new categories:
- `_LIBCPP_ASSERT_VALID_DEALLOCATION`;
- `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL`.
---
Full diff: https://github.com/llvm/llvm-project/pull/77164.diff
6 Files Affected:
- (modified) libcxx/include/__config (+16)
- (modified) libcxx/include/__coroutine/coroutine_handle.h (+8-8)
- (modified) libcxx/include/__memory_resource/polymorphic_allocator.h (+2-1)
- (modified) libcxx/src/filesystem/operations.cpp (+5-3)
- (modified) libcxx/src/memory_resource.cpp (+2-1)
- (modified) libcxx/src/mutex.cpp (+5-3)
``````````diff
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 082c73e672c749..4d74b564864272 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -280,6 +280,14 @@
// - `_LIBCPP_ASSERT_NON_OVERLAPPING_RANGES` -- for functions that take several ranges as arguments, checks that the
// given ranges do not overlap.
//
+// - `_LIBCPP_ASSERT_VALID_DEALLOCATION` -- checks that an attempt to deallocate memory is valid (e.g. the given object
+// was allocated by the given allocator). Violating this category typically results in a memory leak.
+//
+// - `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL` -- checks that a call to an external API (e.g. a syscall) doesn't fail in
+// an unexpected manner. This includes triggering documented cases of undefined behavior in an external library (like
+// attempting to unlock an unlocked mutex in pthreads). We generally don't expect these failures to compromize memory
+// safety or otherwise create an immediate security issue.
+//
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
// the containers have compatible allocators.
//
@@ -327,6 +335,8 @@ _LIBCPP_HARDENING_MODE_DEBUG
// Overlapping ranges will make algorithms produce incorrect results but don't directly lead to a security
// vulnerability.
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
@@ -341,6 +351,8 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
@@ -356,6 +368,8 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message)
@@ -370,6 +384,8 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_VALID_DEALLOCATION(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
diff --git a/libcxx/include/__coroutine/coroutine_handle.h b/libcxx/include/__coroutine/coroutine_handle.h
index 54bfe5b44f4c6a..4557a6643c2393 100644
--- a/libcxx/include/__coroutine/coroutine_handle.h
+++ b/libcxx/include/__coroutine/coroutine_handle.h
@@ -55,7 +55,7 @@ struct _LIBCPP_TEMPLATE_VIS coroutine_handle<void> {
_LIBCPP_HIDE_FROM_ABI constexpr explicit operator bool() const noexcept { return __handle_ != nullptr; }
_LIBCPP_HIDE_FROM_ABI bool done() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "done() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "done() can be called only on suspended coroutines");
return __builtin_coro_done(__handle_);
}
@@ -63,13 +63,13 @@ struct _LIBCPP_TEMPLATE_VIS coroutine_handle<void> {
_LIBCPP_HIDE_FROM_ABI void operator()() const { resume(); }
_LIBCPP_HIDE_FROM_ABI void resume() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "resume() can be called only on suspended coroutines");
- _LIBCPP_ASSERT_UNCATEGORIZED(!done(), "resume() has undefined behavior when the coroutine is done");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "resume() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(!done(), "resume() has undefined behavior when the coroutine is done");
__builtin_coro_resume(__handle_);
}
_LIBCPP_HIDE_FROM_ABI void destroy() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "destroy() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "destroy() can be called only on suspended coroutines");
__builtin_coro_destroy(__handle_);
}
@@ -130,7 +130,7 @@ struct _LIBCPP_TEMPLATE_VIS coroutine_handle {
_LIBCPP_HIDE_FROM_ABI constexpr explicit operator bool() const noexcept { return __handle_ != nullptr; }
_LIBCPP_HIDE_FROM_ABI bool done() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "done() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "done() can be called only on suspended coroutines");
return __builtin_coro_done(__handle_);
}
@@ -138,13 +138,13 @@ struct _LIBCPP_TEMPLATE_VIS coroutine_handle {
_LIBCPP_HIDE_FROM_ABI void operator()() const { resume(); }
_LIBCPP_HIDE_FROM_ABI void resume() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "resume() can be called only on suspended coroutines");
- _LIBCPP_ASSERT_UNCATEGORIZED(!done(), "resume() has undefined behavior when the coroutine is done");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "resume() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(!done(), "resume() has undefined behavior when the coroutine is done");
__builtin_coro_resume(__handle_);
}
_LIBCPP_HIDE_FROM_ABI void destroy() const {
- _LIBCPP_ASSERT_UNCATEGORIZED(__is_suspended(), "destroy() can be called only on suspended coroutines");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__is_suspended(), "destroy() can be called only on suspended coroutines");
__builtin_coro_destroy(__handle_);
}
diff --git a/libcxx/include/__memory_resource/polymorphic_allocator.h b/libcxx/include/__memory_resource/polymorphic_allocator.h
index 3a2794931767b2..a145b0f0371693 100644
--- a/libcxx/include/__memory_resource/polymorphic_allocator.h
+++ b/libcxx/include/__memory_resource/polymorphic_allocator.h
@@ -68,7 +68,8 @@ class _LIBCPP_AVAILABILITY_PMR _LIBCPP_TEMPLATE_VIS polymorphic_allocator {
}
_LIBCPP_HIDE_FROM_ABI void deallocate(_ValueType* __p, size_t __n) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__n <= __max_size(), "deallocate called for size which exceeds max_size()");
+ // This would cause an integer overflow, resulting in too few objects being deleted.
+ _LIBCPP_ASSERT_VALID_DEALLOCATION(__n <= __max_size(), "deallocate called for size which exceeds max_size()");
__res_->deallocate(__p, __n * sizeof(_ValueType), alignof(_ValueType));
}
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 6bee340e0d15c8..0febb0c68bacee 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -461,7 +461,7 @@ path __current_path(error_code* ec) {
Deleter deleter = &::free;
#else
auto size = ::pathconf(".", _PC_PATH_MAX);
- _LIBCPP_ASSERT_UNCATEGORIZED(size >= 0, "pathconf returned a 0 as max size");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(size >= 0, "pathconf returned a 0 as max size");
auto buff = unique_ptr<path::value_type[]>(new path::value_type[size + 1]);
path::value_type* ptr = buff.get();
@@ -621,7 +621,7 @@ void __permissions(const path& p, perms prms, perm_options opts, error_code* ec)
set_sym_perms = is_symlink(st);
if (m_ec)
return err.report(m_ec);
- _LIBCPP_ASSERT_UNCATEGORIZED(st.permissions() != perms::unknown, "Permissions unexpectedly unknown");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(st.permissions() != perms::unknown, "Permissions unexpectedly unknown");
if (add_perms)
prms |= st.permissions();
else if (remove_perms)
@@ -668,7 +668,9 @@ path __read_symlink(const path& p, error_code* ec) {
detail::SSizeT ret;
if ((ret = detail::readlink(p.c_str(), buff.get(), size)) == -1)
return err.report(capture_errno());
- _LIBCPP_ASSERT_UNCATEGORIZED(ret > 0, "TODO");
+ // `ret` indicates the number of bytes written to the buffer, `0` means that the attempt to read the symlink produced
+ // an empty string.
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(ret > 0, "TODO");
if (static_cast<size_t>(ret) >= size)
return err.report(errc::value_too_large);
buff[ret] = 0;
diff --git a/libcxx/src/memory_resource.cpp b/libcxx/src/memory_resource.cpp
index 42c366893f736b..2117238e63487e 100644
--- a/libcxx/src/memory_resource.cpp
+++ b/libcxx/src/memory_resource.cpp
@@ -189,7 +189,8 @@ void unsynchronized_pool_resource::__adhoc_pool::__do_deallocate(
return;
}
}
- _LIBCPP_ASSERT_UNCATEGORIZED(false, "deallocating a block that was not allocated with this allocator");
+ // The request to deallocate memory ends up being a no-op, likely resulting in a memory leak.
+ _LIBCPP_ASSERT_VALID_DEALLOCATION(false, "deallocating a block that was not allocated with this allocator");
}
}
diff --git a/libcxx/src/mutex.cpp b/libcxx/src/mutex.cpp
index ce854757ac08da..2f8504d602dc9f 100644
--- a/libcxx/src/mutex.cpp
+++ b/libcxx/src/mutex.cpp
@@ -36,7 +36,8 @@ bool mutex::try_lock() noexcept { return __libcpp_mutex_trylock(&__m_); }
void mutex::unlock() noexcept {
int ec = __libcpp_mutex_unlock(&__m_);
(void)ec;
- _LIBCPP_ASSERT_UNCATEGORIZED(ec == 0, "call to mutex::unlock failed");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(
+ ec == 0, "call to mutex::unlock failed. A possible reason is that the mutex wasn't locked");
}
// recursive_mutex
@@ -50,7 +51,7 @@ recursive_mutex::recursive_mutex() {
recursive_mutex::~recursive_mutex() {
int e = __libcpp_recursive_mutex_destroy(&__m_);
(void)e;
- _LIBCPP_ASSERT_UNCATEGORIZED(e == 0, "call to ~recursive_mutex() failed");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(e == 0, "call to ~recursive_mutex() failed");
}
void recursive_mutex::lock() {
@@ -62,7 +63,8 @@ void recursive_mutex::lock() {
void recursive_mutex::unlock() noexcept {
int e = __libcpp_recursive_mutex_unlock(&__m_);
(void)e;
- _LIBCPP_ASSERT_UNCATEGORIZED(e == 0, "call to recursive_mutex::unlock() failed");
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(
+ e == 0, "call to recursive_mutex::unlock() failed. A possible reason is that the mutex wasn't locked");
}
bool recursive_mutex::try_lock() noexcept { return __libcpp_recursive_mutex_trylock(&__m_); }
``````````
</details>
https://github.com/llvm/llvm-project/pull/77164
More information about the libcxx-commits
mailing list