[libc++][hardening] Classify assertions related to leaks and syscalls. #77164

var-const · 2024-01-06T00:53:09Z

Introduce two new categories:

_LIBCPP_ASSERT_VALID_DEALLOCATION;
_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL.

Introduce two new categories: - `_LIBCPP_ASSERT_VALID_DEALLOCATION`; - `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL`.

llvmbot · 2024-01-06T00:53:43Z

@llvm/pr-subscribers-libcxx

Author: Konstantin Varlamov (var-const)

Changes

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 --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_); }

var-const · 2024-01-06T00:57:32Z

libcxx/include/__coroutine/coroutine_handle.h

@@ -55,21 +55,21 @@ 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");


This might be a little weird, but to me calling these coroutine compiler built-ins does seem like doing a syscall, even though technically both the library and the compiler are part of the "implementation". It's essentially a black box that we don't control and I don't think we can easily say what happens if this condition is violated (without reading Clang sources, but from our perspective it's no different from reading e.g. pthread sources). I'd prefer not to multiply the number of categories too much and would certainly like to avoid making a separate category just for coroutines. Let me know what you think!

I agree it might look a bit odd, but you could call it an external API. I don't mind to keep it in the same category, but I would like to specifically mention that compiler intrinsics are part of this category.

I agree, I think I would mention that we consider anything external to the library (including compiler builtins) as an "external API" for the purpose of this category.

Good points, done.

var-const · 2024-01-06T00:58:34Z

libcxx/src/filesystem/operations.cpp

@@ -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");


I'm not sure why we treat this as an error -- isn't it valid (if weird) for a user to set permissions to "unknown"? Or is there some context here that I'm missing that should guarantee that the permissions cannot be unknown?

It seems possible -- you could have SELinux enabled that allows the owner of the process to read the file but denies it getattr?

Added a TODO to revisit this later.

var-const · 2024-01-06T00:59:31Z

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");


This condition is suspicious -- the comment seems to indicate it should be size > 0. Also, doesn't 0 indicate "no limit"? (I don't know if any platform can actually report that, though)

Here is something interesting that I found from the man page:

• If name corresponds to a maximum or minimum limit, and that limit is indeterminate, -1 is returned and errno is not changed. (To distin‐ guish an indeterminate limit from an error, set errno to zero before the call, and then check whether errno is nonzero when -1 is re‐ turned.)

So, perhaps, there could be a platform out there that has an indeterminate maximum path length that could return -1 and mean that it was not an error?

My reading of pathconf's manpage is that it returns -1 if there's an error or if there's no limit associated to _PC_PATH_MAX. So I think checking for >= 0 here is correct (although in reality I don't think any system would return 0).

After consideration, it sounds like we should instead be reporting the error if we encounter one? So instead of the assertion, something like

errno = 0; // POSIX requires this to be thread-safe auto size = ::pathconf(".", _PC_PATH_MAX); if (size == -1 && errno != 0) return err.report(capture_errno(), "call to pathconf failed");

If size == -1 but there was no error, the implementation doesn't have a limit for the path length. We should probably set size = PATH_MAX (defined in <limits.h>) in that case. That's a bit academic but it seems reasonable.

CC @EricWF since he most likely wrote that code in the original filesystem implementation.

@ldionne Your reading seems to match with the reading that I had from the man page and the section that I quoted! I am glad that we are seeing the same thing!

I removed the assertion in favor of error reporting. There's an additional quirk that apparently PATH_MAX might not be available on z/OS which I presume false under the #else branch of the #if defined(_LIBCPP_WIN32API) || defined(__GLIBC__) || defined(__APPLE__) condition (it really comes down to the __GLIBC__ define -- I don't know if lack of glibc can be used as a reliable indicator of z/OS).

var-const · 2024-01-06T01:06:52Z

libcxx/src/filesystem/operations.cpp

-  _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");


Something I wanted to point out about the new valid-api-call category is that this is (so far, at least) the only category that also covers spurious failures. All other categories are (or are supposed to be) completely deterministic -- they are direct consequences of the user giving us invalid arguments. System calls, however, can (at least in theory) fail sporadically, with no fault on the user's part.

I'd like to make sure we think it's reasonable. Alternatively, we could:

only leave checks that we only expect to fail due to bad user input that is hard to check. A failure to lock a mutex might be a good example -- it's most likely to happen if the user didn't lock the mutex in the first place (but we don't have an easy way to check for that, so we have to settle for checking the consequences). However, even this category still might cover spurious errors;

separate the user-triggered failures and the spurious failures. For the latter, either create a new category or mark them internal. The downsides are a) arguably too fine a granularity; b) the separation between what is triggered by a user and what is spurious isn't completely deterministic.

I also wonder how often these can trigger when an end-user deletes a file the application was using. AFAIK we don't enable these assertions in the dylib so at the moment they are unchecked. Right?

I also how many of these can be solved by throwing an exception. Obviously that requires validating the functions against the WP.

The only case to consider here is if ret == 0, and I think in that case the code should "work fine". We'll return a buffer of size at least 1, and where buf[0] == '\0'. That's weird but valid. In that line of thought, I think this assertion is wrong (or at least it's in the wrong place).

Removed the assertion.

mordante · 2024-01-07T13:00:02Z

libcxx/include/__config

+//   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.
+//


Not specific to this commit, but I feel this list is getting a bit long in here. How about putting it in an .rst file. I also would like a better overview what the modi do. For example, what does _LIBCPP_HARDENING_MODE_FAST do.
Maybe a summary lke

* _LIBCPP_HARDENING_MODE_FAST * foo * bar * _LIBCPP_HARDENING_MODE_EXTENSIVE * all of _LIBCPP_HARDENING_MODE_FAST * do * re * mi

I completely agree, and I plan to improve the documentation after this round of categorization patches lands.

That probably happens after branching LLVM 18. After branching we can always backport documentation changes. So it would be great if we can get better documentation in LLVM 18.

mordante · 2024-01-07T13:03:23Z

libcxx/include/__coroutine/coroutine_handle.h

@@ -55,21 +55,21 @@ 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");


I agree it might look a bit odd, but you could call it an external API. I don't mind to keep it in the same category, but I would like to specifically mention that compiler intrinsics are part of this category.

mordante · 2024-01-07T13:15:46Z

libcxx/src/filesystem/operations.cpp

-  _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");


I also wonder how often these can trigger when an end-user deletes a file the application was using. AFAIK we don't enable these assertions in the dylib so at the moment they are unchecked. Right?

I also how many of these can be solved by throwing an exception. Obviously that requires validating the functions against the WP.

libcxx/include/__memory_resource/polymorphic_allocator.h

ldionne · 2024-01-09T20:43:52Z

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");


My reading of pathconf's manpage is that it returns -1 if there's an error or if there's no limit associated to _PC_PATH_MAX. So I think checking for >= 0 here is correct (although in reality I don't think any system would return 0).

After consideration, it sounds like we should instead be reporting the error if we encounter one? So instead of the assertion, something like

errno = 0; // POSIX requires this to be thread-safe auto size = ::pathconf(".", _PC_PATH_MAX); if (size == -1 && errno != 0) return err.report(capture_errno(), "call to pathconf failed");

If size == -1 but there was no error, the implementation doesn't have a limit for the path length. We should probably set size = PATH_MAX (defined in <limits.h>) in that case. That's a bit academic but it seems reasonable.

CC @EricWF since he most likely wrote that code in the original filesystem implementation.

ldionne · 2024-01-09T20:57:58Z

libcxx/src/filesystem/operations.cpp

-  _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");


The only case to consider here is if ret == 0, and I think in that case the code should "work fine". We'll return a buffer of size at least 1, and where buf[0] == '\0'. That's weird but valid. In that line of thought, I think this assertion is wrong (or at least it's in the wrong place).

ldionne · 2024-01-09T21:02:09Z

libcxx/include/__coroutine/coroutine_handle.h

@@ -55,21 +55,21 @@ 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");


I agree, I think I would mention that we consider anything external to the library (including compiler builtins) as an "external API" for the purpose of this category.

github-actions · 2024-01-20T01:30:30Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ral `assert.*` pattern

mordante

LGTM modulo one nit.

mordante · 2024-01-21T13:03:14Z

libcxx/include/__config

+//   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.
+//


That probably happens after branching LLVM 18. After branching we can always backport documentation changes. So it would be great if we can get better documentation in LLVM 18.

...s/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/assert.deallocate.pass.cpp

ldionne

LGTM.

[libc++][hardening] Classify assertions related to leaks and syscalls.

e28e7b3

Introduce two new categories: - `_LIBCPP_ASSERT_VALID_DEALLOCATION`; - `_LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL`.

var-const requested a review from a team as a code owner January 6, 2024 00:53

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 6, 2024

var-const commented Jan 6, 2024

View reviewed changes

var-const assigned ldionne Jan 6, 2024

var-const added the hardening Issues related to the hardening effort label Jan 6, 2024

mordante reviewed Jan 7, 2024

View reviewed changes

ldionne reviewed Jan 9, 2024

View reviewed changes

ldionne added this to the LLVM 18.0.X Release milestone Jan 16, 2024

var-const added 2 commits January 19, 2024 17:27

Feedback

c6fb1de

Merge branch 'main' into varconst/hardening-categorization-4

279656b

var-const added 3 commits January 19, 2024 17:32

clang-format

1ff875d

Update error message in a test and rename the test to follow the gene…

43672c3

…ral `assert.*` pattern

Merge branch 'main' into varconst/hardening-categorization-4

63964cb

mordante approved these changes Jan 21, 2024

View reviewed changes

ldionne approved these changes Jan 22, 2024

View reviewed changes

Undo accidental change

5ddc30f

var-const merged commit 6082478 into llvm:main Jan 23, 2024
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][hardening] Classify assertions related to leaks and syscalls. #77164

[libc++][hardening] Classify assertions related to leaks and syscalls. #77164

var-const commented Jan 6, 2024

llvmbot commented Jan 6, 2024

var-const Jan 6, 2024

mordante Jan 7, 2024

ldionne Jan 9, 2024

var-const Jan 20, 2024

var-const Jan 6, 2024

hawkinsw Jan 6, 2024

var-const Jan 20, 2024

var-const Jan 6, 2024

hawkinsw Jan 6, 2024

ldionne Jan 9, 2024

hawkinsw Jan 9, 2024

var-const Jan 20, 2024

var-const Jan 6, 2024

mordante Jan 7, 2024

ldionne Jan 9, 2024

var-const Jan 20, 2024

mordante Jan 7, 2024

var-const Jan 20, 2024

mordante Jan 21, 2024

mordante Jan 7, 2024

mordante Jan 7, 2024

ldionne Jan 9, 2024

ldionne Jan 9, 2024

ldionne Jan 9, 2024

github-actions bot commented Jan 20, 2024 •

edited

mordante left a comment

mordante Jan 21, 2024

ldionne left a comment

[libc++][hardening] Classify assertions related to leaks and syscalls. #77164

[libc++][hardening] Classify assertions related to leaks and syscalls. #77164

Conversation

var-const commented Jan 6, 2024

llvmbot commented Jan 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2024 • edited

mordante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2024 •

edited