[libcxx-commits] [libcxx] [libc++][hardening] In production hardening modes, trap rather than abort (PR #78561)
via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 19 03:21:37 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
Author: Konstantin Varlamov (var-const)
<details>
<summary>Changes</summary>
In the hardening modes that can be used in production (`fast` and `extensive`), make a failed assertion invoke a trap instruction rather than calling verbose abort. In the debug mode, still keep calling verbose abort to provide a better user experience and to allow us to keep our existing testing infrastructure for verifying assertion messages. Since the debug mode by definition enables all assertions, we can be sure that we still check all the assertion messages in the library when running the test suite in the debug mode.
The main motivation to use trapping in production is to achieve better code generation and reduce the binary size penalty. This way, the assertion handler can compile to a single instruction, whereas the existing mechanism with verbose abort results in generating a function call that in general cannot be optimized away (made worse by the fact that it's a variadic function, imposing an additional penalty). See the [RFC](https://discourse.llvm.org/t/rfc-hardening-in-libc/73925) for more details. Note that this mechanism can now be completely [overridden at CMake configuration time](https://github.com/llvm/llvm-project/pull/77883).
This patch also significantly refactors `check_assertion.h` and expands its test coverage. The main changes:
- when overriding `verbose_abort`, don't do matching inside the function -- just print the error message to `stderr`. This removes the need to set a global matcher and allows to do matching in the parent process after the child finishes;
- remove unused logic for matching source locations and for using wildcards;
- make matchers simple functors;
- introduce `DeathTestResult` that keeps data about the test run, primarily to make it easier to test.
In addition to the refactoring, `check_assertion.h` can now recognize when a process exits due to a trap.
---
Patch is 31.54 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78561.diff
8 Files Affected:
- (modified) libcxx/docs/BuildingLibcxx.rst (+8-12)
- (modified) libcxx/docs/ReleaseNotes/18.rst (+14-7)
- (modified) libcxx/test/libcxx/assertions/customize_verbose_abort.compile-time.pass.cpp (+1-1)
- (modified) libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp (+1-1)
- (modified) libcxx/test/libcxx/assertions/default_verbose_abort.pass.cpp (+2-1)
- (modified) libcxx/test/support/check_assertion.h (+225-174)
- (modified) libcxx/test/support/test.support/test_check_assertion.pass.cpp (+96-32)
- (modified) libcxx/vendor/llvm/default_assertion_handler.in (+9-2)
``````````diff
diff --git a/libcxx/docs/BuildingLibcxx.rst b/libcxx/docs/BuildingLibcxx.rst
index 11e250e3f3735a..9a250e7cd79059 100644
--- a/libcxx/docs/BuildingLibcxx.rst
+++ b/libcxx/docs/BuildingLibcxx.rst
@@ -485,18 +485,14 @@ LLVM-specific options
.. _assertion-handler:
Overriding the default assertion handler
-==========================================
-
-When the library wants to terminate due to an unforeseen condition (such as
-a hardening assertion failure), the program is aborted through a special verbose
-termination function. The library provides a default function that prints an
-error message and calls ``std::abort()``. Note that this function is provided by
-the static or shared library, so it is only available when deploying to
-a platform where the compiled library is sufficiently recent. On older
-platforms, the program will terminate in an unspecified unsuccessful manner, but
-the quality of diagnostics won't be great.
-
-However, vendors can also override that mechanism at CMake configuration time.
+========================================
+
+When the library wants to terminate due to a hardening assertion failure, the
+program is aborted by invoking a trap instruction (or in debug mode, by
+a special verbose termination function that prints an error message and calls
+``std::abort()``). However, vendors can also override that mechanism at CMake
+configuration time.
+
When a hardening assertion fails, the library invokes the
``_LIBCPP_ASSERTION_HANDLER`` macro. A vendor may provide a header that contains
a custom definition of this macro and specify the path to the header via the
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index 77b7939a0c0ac9..9cc024c3d7634e 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -111,13 +111,18 @@ Deprecations and Removals
macro is provided to restore the previous behavior, and it will be supported in the LLVM 18 release only.
In LLVM 19 and beyond, ``_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT`` will not be honored anymore.
-- The only supported way to customize the assertion handler that gets invoked when a hardening assertion fails
- is now by setting the ``LIBCXX_ASSERTION_HANDLER_FILE`` CMake variable and providing a custom header. See
- the documentation on overriding the default assertion handler for details.
+- Overriding `__libcpp_verbose_abort` no longer has any effect on library assertions. The only supported way
+ to customize the assertion handler that gets invoked when a hardening assertion fails is now by setting the
+ ``LIBCXX_ASSERTION_HANDLER_FILE`` CMake variable and providing a custom header. See the documentation on
+ overriding the default assertion handler for details. The ability to override `__libcpp_verbose_abort` will
+ be removed in an upcoming release in favor of the new overriding mechanism.
+
+- In safe mode (which is now equivalent to the ``extensive`` hardening mode), a failed assertion will now
+ generate a trap rather than a call to verbose abort.
- The ``_LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED`` macro is not honored anymore in LLVM 18.
- Please see the updated documentation about the hardening modes in libc++ and in particular the
- ``_LIBCPP_VERBOSE_ABORT`` macro for details.
+ Please see the updated documentation about the hardening modes in libc++ and in particular on
+ overriding the default assertion handler.
- The headers ``<experimental/deque>``, ``<experimental/forward_list>``, ``<experimental/list>``,
``<experimental/map>``, ``<experimental/memory_resource>``, ``<experimental/regex>``, ``<experimental/set>``,
@@ -140,13 +145,15 @@ Deprecations and Removals
Upcoming Deprecations and Removals
----------------------------------
+- ``__libcpp_verbose_abort`` and the ability to override this function will be removed in an upcoming release.
+
LLVM 19
~~~~~~~
- The ``LIBCXX_ENABLE_ASSERTIONS`` CMake variable that was used to enable the safe mode will be deprecated and setting
it will trigger an error; use the ``LIBCXX_HARDENING_MODE`` variable with the value ``extensive`` instead. Similarly,
- the ``_LIBCPP_ENABLE_ASSERTIONS`` macro will be deprecated (setting it to ``1`` still enables the extensive mode the
- LLVM 19 release while also issuing a deprecation warning). See :ref:`the hardening documentation
+ the ``_LIBCPP_ENABLE_ASSERTIONS`` macro will be deprecated (setting it to ``1`` still enables the extensive mode in
+ the LLVM 19 release while also issuing a deprecation warning). See :ref:`the hardening documentation
<using-hardening-modes>` for more details.
- The base template for ``std::char_traits`` has been marked as deprecated and will be removed in LLVM 19. If you
diff --git a/libcxx/test/libcxx/assertions/customize_verbose_abort.compile-time.pass.cpp b/libcxx/test/libcxx/assertions/customize_verbose_abort.compile-time.pass.cpp
index d09881e8cecb9f..69154c3f7eaffa 100644
--- a/libcxx/test/libcxx/assertions/customize_verbose_abort.compile-time.pass.cpp
+++ b/libcxx/test/libcxx/assertions/customize_verbose_abort.compile-time.pass.cpp
@@ -22,6 +22,6 @@ void my_abort(char const*, ...) {
}
int main(int, char**) {
- _LIBCPP_ASSERT(false, "message");
+ _LIBCPP_VERBOSE_ABORT("%s", "message");
return EXIT_FAILURE;
}
diff --git a/libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp b/libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp
index 219c43874e77db..585ab73f2cb261 100644
--- a/libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp
+++ b/libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp
@@ -19,6 +19,6 @@ void std::__libcpp_verbose_abort(char const*, ...) {
}
int main(int, char**) {
- _LIBCPP_ASSERT(false, "message");
+ std::__libcpp_verbose_abort("%s", "message");
return EXIT_FAILURE;
}
diff --git a/libcxx/test/libcxx/assertions/default_verbose_abort.pass.cpp b/libcxx/test/libcxx/assertions/default_verbose_abort.pass.cpp
index 870f43da4b8f02..18cc7a9521e498 100644
--- a/libcxx/test/libcxx/assertions/default_verbose_abort.pass.cpp
+++ b/libcxx/test/libcxx/assertions/default_verbose_abort.pass.cpp
@@ -8,6 +8,7 @@
// Test that the default verbose termination function aborts the program.
+#include <__verbose_abort>
#include <csignal>
#include <cstdlib>
@@ -19,6 +20,6 @@ void signal_handler(int signal) {
int main(int, char**) {
if (std::signal(SIGABRT, signal_handler) != SIG_ERR)
- _LIBCPP_ASSERT(false, "foo");
+ std::__libcpp_verbose_abort("%s", "foo");
return EXIT_FAILURE;
}
diff --git a/libcxx/test/support/check_assertion.h b/libcxx/test/support/check_assertion.h
index 83a46548fa9250..485f8103c7ad8d 100644
--- a/libcxx/test/support/check_assertion.h
+++ b/libcxx/test/support/check_assertion.h
@@ -15,7 +15,9 @@
#include <cstdio>
#include <cstdlib>
#include <exception>
+#include <functional>
#include <regex>
+#include <sstream>
#include <string>
#include <string_view>
#include <utility>
@@ -27,118 +29,206 @@
#include "test_allocator.h"
#if TEST_STD_VER < 11
-# error "C++11 or greater is required to use this header"
+# error "C++11 or greater is required to use this header"
#endif
-struct AssertionInfoMatcher {
- static const int any_line = -1;
- static constexpr const char* any_file = "*";
- static constexpr const char* any_msg = "*";
+// When printing the assertion message to `stderr`, delimit it with a marker to make it easier to match the message
+// later.
+static constexpr const char* Marker = "###";
- constexpr AssertionInfoMatcher() : is_empty_(true), msg_(any_msg, __builtin_strlen(any_msg)), file_(any_file, __builtin_strlen(any_file)), line_(any_line) { }
- constexpr AssertionInfoMatcher(const char* msg, const char* file = any_file, int line = any_line)
- : is_empty_(false), msg_(msg, __builtin_strlen(msg)), file_(file, __builtin_strlen(file)), line_(line) {}
+// (success, error-message-if-failed)
+using MatchResult = std::pair<bool, std::string>;
+using Matcher = std::function<MatchResult(const std::string& /*text*/)>;
- bool Matches(char const* file, int line, char const* message) const {
- assert(!empty() && "empty matcher");
+MatchResult MatchAssertionMessage(const std::string& text, std::string_view expected_message) {
+ // Extract information from the error message. This has to stay synchronized with how we format assertions in the
+ // library.
+ std::regex assertion_format(".*###\\n(.*):(\\d+): assertion (.*) failed: (.*)\\n###");
- if (CheckLineMatches(line) && CheckFileMatches(file) && CheckMessageMatches(message))
- return true;
- // Write to stdout because that's the file descriptor captured by the parent
- // process.
- std::printf("Failed to match assertion info!\n%s\nVS\n%s:%d (%s)\n", ToString().data(), file, line, message);
- return false;
+ std::smatch match_result;
+ bool has_match = std::regex_match(text, match_result, assertion_format);
+ assert(has_match);
+ assert(match_result.size() == 5);
+
+ const std::string& file = match_result[1];
+ int line = std::stoi(match_result[2]);
+ // Omitting `expression` in `match_result[3]`
+ const std::string& assertion_message = match_result[4];
+
+ bool result = assertion_message == expected_message;
+ if (!result) {
+ std::stringstream matching_error;
+ matching_error //
+ << "Expected message: '" << expected_message.data() << "'\n" //
+ << "Actual message: '" << assertion_message.c_str() << "'\n" //
+ << "Source location: " << file << ":" << std::to_string(line) << "\n";
+ return MatchResult(/*success=*/false, matching_error.str());
}
- std::string ToString() const {
- std::string result = "msg = \""; result += msg_; result += "\"\n";
- result += "line = " + (line_ == any_line ? "'*'" : std::to_string(line_)) + "\n";
- result += "file = " + (file_ == any_file ? "'*'" : std::string(file_));
- return result;
+ return MatchResult(/*success=*/true, /*maybe_error=*/"");
+}
+
+Matcher MakeAssertionMessageMatcher(std::string_view assertion_message) {
+ return [=](const std::string& text) { //
+ return MatchAssertionMessage(text, assertion_message);
+ };
+}
+
+Matcher MakeAnyMatcher() {
+ return [](const std::string&) { //
+ return MatchResult(/*success=*/true, /*maybe_error=*/"");
+ };
+}
+
+enum class DeathCause {
+ // Valid causes
+ VerboseAbort = 1,
+ StdTerminate,
+ Trap,
+ // Invalid causes
+ DidNotDie,
+ SetupFailure,
+ Unknown
+};
+
+bool IsValidCause(DeathCause cause) {
+ switch (cause) {
+ case DeathCause::VerboseAbort:
+ case DeathCause::StdTerminate:
+ case DeathCause::Trap:
+ return true;
+ default:
+ return false;
}
+}
- bool empty() const { return is_empty_; }
-private:
- bool CheckLineMatches(int got_line) const {
- if (line_ == any_line)
- return true;
- return got_line == line_;
+std::string ToString(DeathCause cause) {
+ switch (cause) {
+ case DeathCause::VerboseAbort:
+ return "verbose abort";
+ case DeathCause::StdTerminate:
+ return "`std::terminate`";
+ case DeathCause::Trap:
+ return "trap";
+ case DeathCause::DidNotDie:
+ return "<invalid cause (child did not die)>";
+ case DeathCause::SetupFailure:
+ return "<invalid cause (child failed to set up test environment)>";
+ case DeathCause::Unknown:
+ return "<invalid cause (cause unknown)>";
}
- bool CheckFileMatches(std::string_view got_file) const {
- assert(!empty() && "empty matcher");
- if (file_ == any_file)
- return true;
- std::size_t found_at = got_file.find(file_);
- if (found_at == std::string_view::npos)
- return false;
- // require the match start at the beginning of the file or immediately after
- // a directory separator.
- if (found_at != 0) {
- char last_char = got_file[found_at - 1];
- if (last_char != '/' && last_char != '\\')
- return false;
- }
- // require the match goes until the end of the string.
- return got_file.substr(found_at) == file_;
+ assert(false && "Unreachable");
+}
+
+TEST_NORETURN void StopChildProcess(DeathCause cause) { std::exit(static_cast<int>(cause)); }
+
+DeathCause ConvertToDeathCause(int val) {
+ if (val < static_cast<int>(DeathCause::VerboseAbort) || val > static_cast<int>(DeathCause::Unknown)) {
+ return DeathCause::Unknown;
}
+ return static_cast<DeathCause>(val);
+}
+
+enum class Outcome {
+ Success,
+ UnexpectedCause,
+ UnexpectedErrorMessage,
+ InvalidCause,
+};
- bool CheckMessageMatches(std::string_view got_msg) const {
- assert(!empty() && "empty matcher");
- if (msg_ == any_msg)
- return true;
- std::size_t found_at = got_msg.find(msg_);
- if (found_at == std::string_view::npos)
- return false;
- return found_at == 0 && got_msg.size() == msg_.size();
+std::string ToString(Outcome outcome) {
+ switch (outcome) {
+ case Outcome::Success:
+ return "success";
+ case Outcome::UnexpectedCause:
+ return "unexpected death cause";
+ case Outcome::UnexpectedErrorMessage:
+ return "unexpected error message";
+ case Outcome::InvalidCause:
+ return "invalid death cause";
}
+
+ assert(false && "Unreachable");
+}
+
+class DeathTestResult {
+public:
+ DeathTestResult() = default;
+ DeathTestResult(Outcome set_outcome, DeathCause set_cause, const std::string& set_failure_description = "")
+ : outcome_(set_outcome), cause_(set_cause), failure_description_(set_failure_description) {}
+
+ bool success() const { return outcome() == Outcome::Success; }
+ Outcome outcome() const { return outcome_; }
+ DeathCause cause() const { return cause_; }
+ const std::string& failure_description() const { return failure_description_; }
+
private:
- bool is_empty_;
- std::string_view msg_;
- std::string_view file_;
- int line_;
+ Outcome outcome_ = Outcome::Success;
+ DeathCause cause_ = DeathCause::Unknown;
+ std::string failure_description_;
};
-static constexpr AssertionInfoMatcher AnyMatcher(AssertionInfoMatcher::any_msg);
+class DeathTest {
+public:
+ DeathTest() = default;
+ DeathTest(DeathTest const&) = delete;
+ DeathTest& operator=(DeathTest const&) = delete;
-inline AssertionInfoMatcher& GlobalMatcher() {
- static AssertionInfoMatcher GMatch;
- return GMatch;
-}
+ template <class Func>
+ DeathTestResult Run(DeathCause expected_cause, Func&& func, const Matcher& matcher) {
+ std::set_terminate([] { StopChildProcess(DeathCause::StdTerminate); });
-struct DeathTest {
- enum ResultKind {
- RK_DidNotDie, RK_MatchFound, RK_MatchFailure, RK_Terminate, RK_SetupFailure, RK_Unknown
- };
+ DeathCause cause = Run(func);
- static const char* ResultKindToString(ResultKind RK) {
-#define CASE(K) case K: return #K
- switch (RK) {
- CASE(RK_MatchFailure);
- CASE(RK_DidNotDie);
- CASE(RK_SetupFailure);
- CASE(RK_MatchFound);
- CASE(RK_Unknown);
- CASE(RK_Terminate);
+ if (!IsValidCause(cause)) {
+ return DeathTestResult(Outcome::InvalidCause, cause, ToString(cause));
}
- return "not a result kind";
- }
- static bool IsValidResultKind(int val) {
- return val >= RK_DidNotDie && val <= RK_Unknown;
+ if (expected_cause != cause) {
+ std::stringstream failure_description;
+ failure_description //
+ << "Child died, but with a different death cause\n" //
+ << "Expected cause: " << ToString(expected_cause) << "\n" //
+ << "Actual cause: " << ToString(cause) << "\n";
+ return DeathTestResult(Outcome::UnexpectedCause, cause, failure_description.str());
+ }
+
+ MatchResult match_result = matcher(GetChildStdErr());
+ if (!match_result.first) {
+ auto failure_description = std::string("Child died, but with a different error message\n") + match_result.second;
+ return DeathTestResult(Outcome::UnexpectedErrorMessage, cause, failure_description);
+ }
+
+ return DeathTestResult(Outcome::Success, cause);
}
- DeathTest(AssertionInfoMatcher const& Matcher) : matcher_(Matcher) {}
+ void PrintFailureDetails(std::string_view failure_description, std::string_view stmt, DeathCause cause) const {
+ std::fprintf(
+ stderr, "Failure: EXPECT_DEATH( %s ) failed!\n(reason: %s)\n\n", stmt.data(), failure_description.data());
+
+ if (cause != DeathCause::Unknown) {
+ std::fprintf(stderr, "child exit code: %d\n", GetChildExitCode());
+ }
+ std::fprintf(stderr, "---------- standard err ----------\n%s", GetChildStdErr().c_str());
+ std::fprintf(stderr, "\n----------------------------------\n");
+ std::fprintf(stderr, "---------- standard out ----------\n%s", GetChildStdOut().c_str());
+ std::fprintf(stderr, "\n----------------------------------\n");
+ };
+
+private:
+ int GetChildExitCode() const { return exit_code_; }
+ std::string const& GetChildStdOut() const { return stdout_from_child_; }
+ std::string const& GetChildStdErr() const { return stderr_from_child_; }
template <class Func>
- ResultKind Run(Func&& f) {
+ DeathCause Run(Func&& f) {
int pipe_res = pipe(stdout_pipe_fd_);
assert(pipe_res != -1 && "failed to create pipe");
pipe_res = pipe(stderr_pipe_fd_);
assert(pipe_res != -1 && "failed to create pipe");
pid_t child_pid = fork();
- assert(child_pid != -1 &&
- "failed to fork a process to perform a death test");
+ assert(child_pid != -1 && "failed to fork a process to perform a death test");
child_pid_ = child_pid;
if (child_pid_ == 0) {
RunForChild(std::forward<Func>(f));
@@ -147,10 +237,6 @@ struct DeathTest {
return RunForParent();
}
- int getChildExitCode() const { return exit_code_; }
- std::string const& getChildStdOut() const { return stdout_from_child_; }
- std::string const& getChildStdErr() const { return stderr_from_child_; }
-private:
template <class Func>
TEST_NORETURN void RunForChild(Func&& f) {
close(GetStdOutReadFD()); // don't need to read from the pipe in the child.
@@ -158,14 +244,13 @@ struct DeathTest {
auto DupFD = [](int DestFD, int TargetFD) {
int dup_result = dup2(DestFD, TargetFD);
if (dup_result == -1)
- std::exit(RK_SetupFailure);
+ StopChildProcess(DeathCause::SetupFailure);
};
DupFD(GetStdOutWriteFD(), STDOUT_FILENO);
DupFD(GetStdErrWriteFD(), STDERR_FILENO);
- GlobalMatcher() = matcher_;
f();
- std::exit(RK_DidNotDie);
+ StopChildProcess(DeathCause::DidNotDie);
}
static std::string ReadChildIOUntilEnd(int FD) {
@@ -190,7 +275,7 @@ struct DeathTest {
close(GetStdErrReadFD());
}
- ResultKind RunForParent() {
+ DeathCause RunForParent() {
CaptureIOFromChild();
int status_value;
@@ -199,35 +284,27 @@ struct DeathTest {
if (WIFEXITED(status_value)) {
exit_code_ = WEXITSTATUS(status_value);
- if (!IsValidResultKind(exit_code_))
- return RK_Unknown;
- return static_cast<ResultKind>(exit_code_);
+ return ConvertToDeathCause(exit_code_);
}
- return RK_Unknown;
- }
- DeathTest(DeathTest const&) = delete;
- DeathTest& operator=(DeathTest const&) = delete;
+ if (WIFSIGNALED(status_value)) {
+ exit_code_ = WTERMSIG(status_value);
+ // `__builtin_trap` generqtes `SIGILL` on x86 and `SIGTRAP` on ARM.
+ if (exit_code_ == SIGILL || exit_code_ == SIGTRAP) {
+ return DeathCause::Trap;
+ }
+ }
- int GetStdOutReadFD() const {
- return stdout_pipe_fd_[0];
+ return DeathCause::Unknown;
}
- int GetStdOutWriteFD() const {
- return stdout_pipe_fd_[1];
- }
+ int GetStdOutReadFD() const { return stdout_pipe_fd_[0]; }
+ int GetStdOutWriteFD() const { return stdout_pipe_fd_[1]; }
+ int GetStdErrReadFD() const { return stderr_pipe_fd_[0]; }
+ int GetStdErrWriteFD() const { return stderr_pipe_fd_[1]; }
- int GetStdErrReadFD() const {
- return stderr_pipe_fd_[0];
- }
-
- int GetStdErrWriteFD() const {
- return stderr_pipe_fd_[1];
- }
-private:
- AssertionInfoMatcher matcher_;
pid_t child_pid_ = -1;
- int exit_code_ = -1;
+ int exit_code_ = -1;
int stdout_pipe_fd_[2];
int stderr_pipe_fd_[2];
std::string stdout_from_child_;
@...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/78561
More information about the libcxx-commits
mailing list