[clang-tools-extra] [libcxx] [llvm] [clang] [libc++][hardening] Classify assertions related to leaks and syscalls. (PR #77164)
Konstantin Varlamov via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 19 17:30:33 PST 2024
https://github.com/var-const updated https://github.com/llvm/llvm-project/pull/77164
>From e28e7b3e1337cb960cdc8028a70a43740fa7d636 Mon Sep 17 00:00:00 2001
From: Konstantin Varlamov <varconsteq at gmail.com>
Date: Thu, 21 Dec 2023 14:36:47 -0800
Subject: [PATCH 1/2] [libc++][hardening] Classify assertions related to leaks
and syscalls.
Introduce two new categories:
- `_LIBCPP_ASSERT_VALID_DEALLOCATION`;
- `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL`.
---
libcxx/include/__config | 16 ++++++++++++++++
libcxx/include/__coroutine/coroutine_handle.h | 16 ++++++++--------
.../__memory_resource/polymorphic_allocator.h | 3 ++-
libcxx/src/filesystem/operations.cpp | 8 +++++---
libcxx/src/memory_resource.cpp | 3 ++-
libcxx/src/mutex.cpp | 8 +++++---
6 files changed, 38 insertions(+), 16 deletions(-)
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_); }
>From c6fb1dea762178e7b850c5f13da9aaa28723eb21 Mon Sep 17 00:00:00 2001
From: Konstantin Varlamov <varconsteq at gmail.com>
Date: Fri, 19 Jan 2024 17:27:04 -0800
Subject: [PATCH 2/2] Feedback
---
libcxx/include/__config | 7 ++++---
.../__memory_resource/polymorphic_allocator.h | 6 ++++--
libcxx/src/filesystem/operations.cpp | 21 +++++++++++++++----
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 4d74b564864272..1da15cf529b4c0 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -283,10 +283,11 @@
// - `_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
+// - `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL` -- checks that a call to an external API 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.
+// attempting to unlock an unlocked mutex in pthreads). Any API external to the library falls under this category
+// (from system calls to compiler intrinsics). 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.
diff --git a/libcxx/include/__memory_resource/polymorphic_allocator.h b/libcxx/include/__memory_resource/polymorphic_allocator.h
index a145b0f0371693..cfd07bc84fe8aa 100644
--- a/libcxx/include/__memory_resource/polymorphic_allocator.h
+++ b/libcxx/include/__memory_resource/polymorphic_allocator.h
@@ -68,8 +68,10 @@ class _LIBCPP_AVAILABILITY_PMR _LIBCPP_TEMPLATE_VIS polymorphic_allocator {
}
_LIBCPP_HIDE_FROM_ABI void deallocate(_ValueType* __p, size_t __n) {
- // 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()");
+ _LIBCPP_ASSERT_VALID_DEALLOCATION(
+ __n <= __max_size(),
+ "deallocate() called for a size which exceeds max_size(), leading to a memory leak "
+ "(the argument will overflow and result in too few objects being deleted)");
__res_->deallocate(__p, __n * sizeof(_ValueType), alignof(_ValueType));
}
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 0febb0c68bacee..8a99347fe122af 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -460,8 +460,21 @@ path __current_path(error_code* ec) {
typedef decltype(&::free) Deleter;
Deleter deleter = &::free;
#else
+ errno = 0; // Note: POSIX mandates that modifying `errno` is thread-safe.
auto size = ::pathconf(".", _PC_PATH_MAX);
- _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(size >= 0, "pathconf returned a 0 as max size");
+ if (size == -1) {
+ if (errno != 0) {
+ return err.report(capture_errno(), "call to pathconf failed");
+
+ // `pathconf` returns `-1` without an error to indicate no limit.
+ } else {
+# if defined(__MVS__) && !defined(PATH_MAX)
+ size = _XOPEN_PATH_MAX + 1;
+# else
+ size = PATH_MAX + 1;
+# endif
+ }
+ }
auto buff = unique_ptr<path::value_type[]>(new path::value_type[size + 1]);
path::value_type* ptr = buff.get();
@@ -621,6 +634,8 @@ 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);
+ // TODO(hardening): double-check this assertion -- it might be a valid (if rare) case when the permissions are
+ // unknown.
_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(st.permissions() != perms::unknown, "Permissions unexpectedly unknown");
if (add_perms)
prms |= st.permissions();
@@ -668,9 +683,7 @@ 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());
- // `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");
+ // Note that `ret` returning `0` would work, resulting in a valid empty string being returned.
if (static_cast<size_t>(ret) >= size)
return err.report(errc::value_too_large);
buff[ret] = 0;
More information about the cfe-commits
mailing list