Skip to content

[libc++][hardening] Rework how the assertion handler can be overridden. #77883

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.

Sign up for GitHub

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

Merged
var-const merged 19 commits into from
Jan 18, 2024
Merged

[libc++][hardening] Rework how the assertion handler can be overridden. #77883

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e8e95c5
[libc++][hardening] Rework how the assertion handler can be overridden.
var-const Jan 12, 2024
86fc352
Pass the original error message as the first argument.
var-const Jan 12, 2024
216605f
Update the documentation about overriding the default assertion handler.
var-const Jan 12, 2024
692aac0
Update the release notes
var-const Jan 12, 2024
8981ef9
Add license to the new headers
var-const Jan 12, 2024
50282f3
Partially address feedback, update how matching the error message works
var-const Jan 13, 2024
deae2b8
Finish addressing feedback
var-const Jan 13, 2024
d38fe7e
- Use the `COPYONLY` option;
var-const Jan 13, 2024
dc9d771
Remove the new header from another script
var-const Jan 13, 2024
d83f5a1
Try to fix the bootstrapping CI job (install the header consistently
var-const Jan 16, 2024
48db22a
Fix the regex
var-const Jan 17, 2024
230de80
Workarounds for the `%` character in asserted expressions.
var-const Jan 17, 2024
5dd7588
Formatting
var-const Jan 17, 2024
b6bcc16
Fix the CI.
var-const Jan 17, 2024
1f5cab9
Mark the tests failing in `no-localization` configuration because of the
var-const Jan 17, 2024
0bef0fb
Poke CI
var-const Jan 17, 2024
cfa7d34
Poke CI
var-const Jan 17, 2024
6cb68e6
Better way to work around the `%` issue
var-const Jan 17, 2024
8cc2fff
Update `check_assertion` to match 2-argument verbose abort
var-const Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libcxx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ if (NOT "${LIBCXX_HARDENING_MODE}" IN_LIST LIBCXX_SUPPORTED_HARDENING_MODES)
message(FATAL_ERROR
"Unsupported hardening mode: '${LIBCXX_HARDENING_MODE}'. Supported values are ${LIBCXX_SUPPORTED_HARDENING_MODES}.")
endif()
set(LIBCXX_ASSERTION_HANDLER_FILE
"${CMAKE_CURRENT_SOURCE_DIR}/vendor/llvm/default_assertion_handler.in"
CACHE STRING
"Specify the path to a header that contains a custom implementation of the
assertion handler that gets invoked when a hardening assertion fails. If
provided, the contents of this header will get injected into the library code
and override the default assertion handler.")
Copy link
Member

@mordante mordante Jan 12, 2024

Choose a reason for hiding this comment

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

Looking at the code it feels somewhat clunky to me.
Why would the following approach not work?

set(LIBCXX_ASSERTION_HANDLER_FILE
  "${CMAKE_CURRENT_SOURCE_DIR}/vendor/llvm/default_assertion_handler.h"
  CACHE STRING
  "Specify the path to a header that contains a custom implementation of the
   assertion handler that gets invoked when a hardening assertion fails. If
   provided, this header will be used instead of libc++'s default handler.")

Then in libcxx/include/CMakeLists.txt you copy this file and no configuring at all.

The current approach would result in a file below or am I missing something.

// -*- C++ -*-
#ifndef _LIBCPP___ASSERTION_HANDLER
#define _LIBCPP___ASSERTION_HANDLER

#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

// -*- C++ -*-
#include <__config>
#include <__verbose_abort>

#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#else

// TODO(hardening): in production, trap rather than abort.
#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#endif // #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#endif // _LIBCPP___ASSERTION_HANDLER

Copy link
Member Author

@var-const var-const Jan 12, 2024

Choose a reason for hiding this comment

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

Injecting the contents of the header should give us a little more control over the custom handler. In particular, it is currently up to the vendor whether they want to allow users to override the macro. To support overriding, a custom implementation might first check that the macro isn't already defined, e.g. on the command line:

// In a vendor-provided `./custom_assertion_handler.h`
#ifndef _LIBCPP_ASSERTION_HANDLER
#define _LIBCPP_ASSERTION_HANDLER(...) platform::assert(__VA_ARGS__)
#endif

That way, a user can compile with -D_LIBCPP_ASSERTION_HANDLER(...)=my_project::special_assert(__VA_ARGS__) and it will be honored. Or the vendor could do the opposite and explicitly forbid overriding:

#ifdef _LIBCPP_ASSERTION_HANDLER
#error "You should not attempt to override the platform-provided assertion handler"
#endif

If in a future release we wanted to make sure that the handler can always be overridden by users, we could change our header to do something like:

#ifndef _LIBCPP_ASSERTION_HANDLER
@_LIBCPP_ASSERTION_HANDLER_OVERRIDE@
#endif

We don't necessarily want to do that, but I think leaving us some degree of flexibility in this regard is useful.

Also, this allows us to provide some internal includes for the custom header in a supported manner. E.g. we can make <__config> available without having the vendor include that internal header explicitly (which we generally don't support), and if in the future we decided to granularize <__config>, we could update the includes without the vendor having to update their code.

Copy link
Member Author

@var-const var-const Jan 13, 2024

Choose a reason for hiding this comment

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

@mordante After writing this, I realized that we can just as well do something like:

#ifndef _LIBCPP_ASSERTION_HANDLER
#include "__custom_assertion_handler.h"
#endif

which alleviates my concern. In fact, I think both approaches have equal expressive power and are not visible to the user/vendor. I think they're very similar, but having a single header seems to be a little simpler/cleaner, so I'll give it a shot based on your suggestion.

option(LIBCXX_ENABLE_RANDOM_DEVICE
"Whether to include support for std::random_device in the library. Disabling
this can be useful when building the library for platforms that don't have
Expand Down
9 changes: 9 additions & 0 deletions libcxx/include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,9 +1019,12 @@ foreach(feature LIBCXX_ENABLE_FILESYSTEM LIBCXX_ENABLE_LOCALIZATION LIBCXX_ENABL
endforeach()

configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
file(READ ${LIBCXX_ASSERTION_HANDLER_FILE} _LIBCPP_ASSERTION_HANDLER_OVERRIDE)
Copy link
Member

@ldionne ldionne Jan 12, 2024

Choose a reason for hiding this comment

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

I think this changes to something like

diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 0fe3ab44d246..7546d8f2aed9 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -1020,9 +1020,11 @@ endforeach()
 
 configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
 configure_file("module.modulemap.in" "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap" @ONLY)
+file(COPY "${LIBCXX_ASSERTION_HANDLER_FILE}" "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
 
 set(_all_includes "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site"
-                  "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap")
+                  "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap"
+                  "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
 foreach(f ${files})
   set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
   set(dst "${LIBCXX_GENERATED_INCLUDE_DIR}/${f}")

after @mordante 's suggestion. It's a bit simpler indeed, we should probably go for that.

configure_file("__assertion_handler.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler" @ONLY)
configure_file("module.modulemap.in" "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap" @ONLY)

set(_all_includes "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site"
"${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler"
"${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap")
foreach(f ${files})
set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
Expand Down Expand Up @@ -1059,6 +1062,12 @@ if (LIBCXX_INSTALL_HEADERS)
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT cxx-headers)

# Install the generated __assertion_handler file to the per-target include dir.
install(FILES "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler"
DESTINATION "${LIBCXX_INSTALL_INCLUDE_TARGET_DIR}"
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT cxx-headers)

# Install the generated modulemap file to the generic include dir.
install(FILES "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap"
DESTINATION "${LIBCXX_INSTALL_INCLUDE_DIR}"
Expand Down
5 changes: 3 additions & 2 deletions libcxx/include/__assert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
#ifndef _LIBCPP___ASSERT
#define _LIBCPP___ASSERT

#include <__assertion_handler>
#include <__config>
#include <__verbose_abort>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand All @@ -20,7 +20,8 @@
#define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \
: _LIBCPP_VERBOSE_ABORT( \
: _LIBCPP_ASSERTION_HANDLER( \
message, \
"%s:%d: assertion %s failed: %s\n", __builtin_FILE(), __builtin_LINE(), #expression, message))

// TODO: __builtin_assume can currently inhibit optimizations. Until this has been fixed and we can add
Expand Down
13 changes: 13 additions & 0 deletions libcxx/include/__assertion_handler.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// -*- C++ -*-
#ifndef _LIBCPP___ASSERTION_HANDLER
#define _LIBCPP___ASSERTION_HANDLER

#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

@_LIBCPP_ASSERTION_HANDLER_OVERRIDE@

#endif // _LIBCPP___ASSERTION_HANDLER
1 change: 1 addition & 0 deletions libcxx/test/libcxx/lint/lint_headers.sh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def exclude_from_consideration(path):
or path.endswith(".modulemap.in")
or os.path.basename(path) == "__config"
or os.path.basename(path) == "__config_site.in"
or os.path.basename(path) == "__assertion_handler.in"
or os.path.basename(path) == "libcxx.imp"
or os.path.basename(path).startswith("__pstl")
or not os.path.isfile(path) # TODO: Remove once PSTL integration is finished
Expand Down
2 changes: 2 additions & 0 deletions libcxx/utils/generate_iwyu_mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ def generate_map(include):
public = []
if i == "__assert":
continue
elif i == "__assertion_handler.in":
continue
elif i == "__availability":
continue
elif i == "__bit_reference":
Expand Down
3 changes: 3 additions & 0 deletions libcxx/utils/libcxx/header_information.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ def is_modulemap_header(header):
if header == "__config_site":
return False

if header == "__assertion_handler":
return False

# exclude libc++abi files
if header in ["cxxabi.h", "__cxxabi_config.h"]:
return False
Expand Down
14 changes: 14 additions & 0 deletions libcxx/vendor/llvm/default_assertion_handler.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// -*- C++ -*-
Copy link
Member Author

@var-const var-const Jan 13, 2024

Choose a reason for hiding this comment

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

Do we want to prefix this filename with __?

Copy link
Member

@ldionne ldionne Jan 15, 2024

Choose a reason for hiding this comment

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

We need to, if it gets installed into $PREFIX/usr/include/c++/v1.

Copy link
Member Author

@var-const var-const Jan 17, 2024

Choose a reason for hiding this comment

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

Sorry, I mean this specific file. During installation, it gets renamed to __assertion_handler, so the generated include directory will contain the prefixed version.

Copy link
Member

@ldionne ldionne Jan 17, 2024

Choose a reason for hiding this comment

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

I guess I would leave it without __, since that makes it more obvious that the two files are named differently.

#include <__config>
#include <__verbose_abort>

#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#else

// TODO(hardening): in production, trap rather than abort.
#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#endif // #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG