Skip to content

DRAFT [libc++][hardening] In production hardening modes, trap rather than abort #78497

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

Closed
wants to merge 17 commits into from

Conversation

var-const
Copy link
Member

  • [libc++][hardening] Rework how the assertion handler can be overridden.
  • Pass the original error message as the first argument.
  • Update the documentation about overriding the default assertion handler.
  • Update the release notes
  • Add license to the new headers
  • Partially address feedback, update how matching the error message works in tests.
  • Finish addressing feedback
    • Use the COPYONLY option; - Reformat tests; - Remove the header from test scripts in cases where it's no longer relevant.
  • Remove the new header from another script
  • Try to fix the bootstrapping CI job (install the header consistently into the generic include directory).
  • Fix the regex
  • Workarounds for the % character in asserted expressions.
  • Formatting
  • Fix the CI.
  • Mark the tests failing in no-localization configuration because of the new regex include as unsupported
  • Wip
  • Wip

Previously there were two ways to override the verbose abort function
which gets called when a hardening assertion is triggered:
- compile-time: define the `_LIBCPP_VERBOSE_ABORT` macro;
- link-time: provide a definition of `__libcpp_verbose_abort` function.

This patch adds a new configure-time approach: a vendor can provide
a path to a custom header file whose contents will get injected into the
build by CMake. The header must provide a definition of the
`_LIBCPP_ASSERTION_HANDLER` macro which is what will get called should
a hardening assertion fail. As of this patch, overriding
`_LIBCPP_VERBOSE_ABORT` will still work, but the previous mechanisms
will be effectively removed in a follow-up patch, making the
configure-time mechanism the sole way of overriding the default handler.

Note that `_LIBCPP_ASSERTION_HANDLER` only gets invoked when a hardening
assertion fails. It does not affect other cases where
`_LIBCPP_VERBOSE_ABORT` is currently used (e.g. when an exception is
thrown in the `-fno-exceptions` mode).

The library provides a default version of the custom header file that
will get used if it's not overridden by the vendor. That allows us to
always test the override mechanism and reduces the difference in
configuration between the pristine version of the library and
a platform-specific version.
- Reformat tests;
- Remove the header from test scripts in cases where it's no longer
  relevant.
@var-const var-const requested a review from a team as a code owner January 17, 2024 20:02
@var-const var-const changed the title [libc++][hardening] In production hardening modes, trap rather than abort DRAFT [libc++][hardening] In production hardening modes, trap rather than abort Jan 17, 2024
@var-const var-const marked this pull request as draft January 17, 2024 20:03
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 4f215fdd62d3f014750339eab9a46946b6fb1c4a 679718ac48a5d78bfdcae090f87c08fe4108d118 -- libcxx/include/__assert libcxx/include/__memory/assume_aligned.h libcxx/src/include/to_chars_floating_point.h libcxx/src/memory_resource.cpp libcxx/test/libcxx/assertions/modes/enabling_assertions_enables_extensive_mode.pass.cpp libcxx/test/libcxx/assertions/modes/override_with_debug_mode.pass.cpp libcxx/test/libcxx/assertions/modes/override_with_extensive_mode.pass.cpp libcxx/test/libcxx/assertions/modes/override_with_fast_mode.pass.cpp libcxx/test/libcxx/assertions/modes/override_with_unchecked_mode.pass.cpp libcxx/test/libcxx/containers/sequences/deque/asan_caterpillar.pass.cpp libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.find/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.sorting/alg.merge/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/numeric.ops/reduce/pstl.exception_handling.pass.cpp libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.exception_handling.pass.cpp libcxx/test/support/check_assertion.h
View the diff from clang-format here.
diff --git a/libcxx/test/support/check_assertion.h b/libcxx/test/support/check_assertion.h
index 6534196956..bbe5027af4 100644
--- a/libcxx/test/support/check_assertion.h
+++ b/libcxx/test/support/check_assertion.h
@@ -58,9 +58,7 @@ struct AssertionInfoMatcher {
   }
 
   bool empty() const { return is_empty_; }
-  bool IsAnyMatcher() const {
-    return msg_ == any_msg && file_ == any_file && line_ == any_line;
-  }
+  bool IsAnyMatcher() const { return msg_ == any_msg && file_ == any_file && line_ == any_line; }
 
 private:
   bool CheckLineMatches(int got_line) const {
@@ -275,11 +273,7 @@ void std::__libcpp_verbose_abort(char const* message, ...) {
   std::exit(DeathTest::RK_Terminate);
 }
 
-enum class DeathCause {
-  VerboseAbort,
-  StdTerminate,
-  HardeningAssertion
-};
+enum class DeathCause { VerboseAbort, StdTerminate, HardeningAssertion };
 
 template <class Func>
 inline bool ExpectDeath(DeathCause expected_cause, const char* stmt, Func&& func, AssertionInfoMatcher matcher) {
@@ -310,15 +304,15 @@ inline bool ExpectDeath(DeathCause expected_cause, const char* stmt, Func&& func
 
   case DeathTest::RK_Trap:
     switch (expected_cause) {
-  case DeathCause::HardeningAssertion:
+    case DeathCause::HardeningAssertion:
 #if _LIBCPP_HARDENING_MODE != _LIBCPP_HARDENING_MODE_DEBUG
-    return true;
+      return true;
 #else
-    return OnFailure("The process has trapped but was expected to invoke verbose abort.");
+      return OnFailure("The process has trapped but was expected to invoke verbose abort.");
 #endif
-  case DeathCause::VerboseAbort:
+    case DeathCause::VerboseAbort:
       return OnFailure("The process has trapped but was expected to invoke verbose abort.");
-  case DeathCause::StdTerminate:
+    case DeathCause::StdTerminate:
       return OnFailure("The process has trapped but was expected to call `std::terminate`.");
     }
 
@@ -341,10 +335,15 @@ inline bool ExpectDeath(DeathCause expected_cause, const char* stmt, Func&& func
 }
 
 /// Assert that the specified expression aborts.
-#define EXPECT_DEATH(...) /*            */ assert((ExpectDeath(DeathCause::VerboseAbort, #__VA_ARGS__, [&]() { __VA_ARGS__; } )))
-#define EXPECT_DEATH_MATCHES(matcher, ...) assert((ExpectDeath(DeathCause::VerboseAbort, #__VA_ARGS__, [&]() { __VA_ARGS__; }, matcher)))
+#define EXPECT_DEATH(...) /*            */                                                                             \
+  assert((ExpectDeath(DeathCause::VerboseAbort, #__VA_ARGS__, [&]() { __VA_ARGS__; })))
+#define EXPECT_DEATH_MATCHES(matcher, ...)                                                                             \
+  assert((ExpectDeath(                                                                                                 \
+      DeathCause::VerboseAbort, #__VA_ARGS__, [&]() { __VA_ARGS__; }, matcher)))
 #define EXPECT_STD_TERMINATE(...) /*    */ assert(ExpectDeath(DeathCause::StdTerminate, #__VA_ARGS__, __VA_ARGS__))
 
-#define TEST_LIBCPP_ASSERT_FAILURE(expr, message) assert((ExpectDeath(DeathCause::HardeningAssertion, #expr, [&]() { (void)(expr); }, AssertionInfoMatcher(message))))
+#define TEST_LIBCPP_ASSERT_FAILURE(expr, message)                                                                      \
+  assert((ExpectDeath(                                                                                                 \
+      DeathCause::HardeningAssertion, #expr, [&]() { (void)(expr); }, AssertionInfoMatcher(message))))
 
 #endif // TEST_SUPPORT_CHECK_ASSERTION_H

template <class Func>
inline bool ExpectDeath(const char* stmt, Func&& func, AssertionInfoMatcher Matcher) {
inline bool ExpectDeath(DeathCause expected_cause, const char* stmt, Func&& func, AssertionInfoMatcher matcher) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest refactoring the code to something like:

enum ResultKind { RK_DidNotDie, RK_Terminate, RK_Trap, RK_VerboseAbort, RK_SetupFailure, RK_Unknown };

Then, the child process would:

  1. Use a std::terminate_handler to exit with RK_Terminate
  2. Override __libcpp_verbose_abort to exit with RK_VerboseAbort and print the message to stderr (but wouldn't do any matching)
  3. Would return RK_DidNotDie otherwise (as it does)

The parent process would:

  1. Check whether the child exited with a trap -- which the child process unfortunately can't signal
  2. In case it terminated with RK_VerboseAbort, match on stderr.

Finally we could do something like

#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
#  define TEST_LIBCPP_ASSERT_FAILURE(expr, message) assert(ExpectDeath(DeathTest::RK_VerboseAbort, ...))
#else
#  define TEST_LIBCPP_ASSERT_FAILURE(expr, message) assert(ExpectDeath(DeathTest::RK_Trap, ...))
#endif

That way you'd have a single enum kind representing all ways the child process can exit, and the error matching would be done in the parent (which IMO makes more sense -- it's always been weird to do it from the child).

@ldionne
Copy link
Member

ldionne commented Jan 18, 2024

This has moved to #78561 now.

@ldionne ldionne closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants