[libc++][hardening] Finish documenting hardening. #92021
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Konstantin Varlamov (var-const) ChangesFull diff: https://github.com/llvm/llvm-project/pull/92021.diff 3 Files Affected:
diff --git a/libcxx/docs/Hardening.rst b/libcxx/docs/Hardening.rst
index 0761f42368e92..59ff8cd84248f 100644
--- a/libcxx/docs/Hardening.rst
+++ b/libcxx/docs/Hardening.rst
@@ -1,4 +1,4 @@
-.. _hardening-modes:
+.. _hardening:
===============
Hardening Modes
@@ -29,8 +29,11 @@ modes are:
rigour impacts performance more than fast mode: we recommend benchmarking to
determine if that is acceptable for your program.
- **Debug mode**, which enables all the available checks in the library,
- including internal assertions, some of which might be very expensive. This
- mode is intended to be used for testing, not in production.
+ including heuristic checks that might have significant performance overhead as
+ well as internal library assertions. This mode should be used in
+ non-production environments (such as test suites, CI, local development). We
+ don’t commit to a particular level of performance in this mode and it’s *not*
+ intended to be used in production.
.. note::
@@ -72,17 +75,301 @@ to control the level by passing **one** of the following options to the compiler
Notes for vendors
-----------------
-Vendors can set the default hardening mode by providing ``LIBCXX_HARDENING_MODE``
-as a configuration option, with the possible values of ``none``, ``fast``,
-``extensive`` and ``debug``. The default value is ``none`` which doesn't enable
-any hardening checks (this mode is sometimes called the ``unchecked`` mode).
+Vendors can set the default hardening mode by providing
+``LIBCXX_HARDENING_MODE`` as a configuration option, with the possible values of
+``none``, ``fast``, ``extensive`` and ``debug``. The default value is ``none``
+which doesn't enable any hardening checks (this mode is sometimes called the
+``unchecked`` mode).
This option controls both the hardening mode that the precompiled library is
built with and the default hardening mode that users will build with. If set to
``none``, the precompiled library will not contain any assertions, and user code
will default to building without assertions.
-Iterator bounds checking
-------------------------
+Vendors can also override the termination handler by :ref:`providing a custom
+header <override-assertion-handler>`.
-TODO(hardening)
+Assertion categories
+====================
+
+Inside the library, individual assertions are grouped into different
+*categories*. Each hardening mode enables a different set of assertion
+categories; categories provide an additional layer of abstraction that makes it
+easier to reason about the high-level semantics of a hardening mode.
+
+- ``valid-element-access`` -- checks that any attempts to access a container
+ element, whether through the container object or through an iterator, are
+ valid and do not attempt to go out of bounds or otherwise access
+ a non-existent element. For iterator checks to work, bounded iterators must be
+ enabled in the ABI. Types like ``optional`` and ``function`` are considered
+ one-element containers for the purposes of this check.
+
+- ``valid-input-range`` -- checks that ranges (whether expressed as an iterator
+ pair, an iterator and a sentinel, an iterator and a count, or
+ a ``std::range``) given as input to library functions are valid:
+ - the sentinel is reachable from the begin iterator;
+ - TODO(hardening): both iterators refer to the same container.
+
+ ("input" here refers to "an input given to an algorithm", not to an iterator
+ category)
+
+ Violating assertions in this category leads to an out-of-bounds access.
+
+- ``non-null`` -- checks that the pointer being dereferenced is not null. On
+ most modern platforms zero address does not refer to an actual location in
+ memory, so a null pointer dereference would not compromize the memory security
+ of a program (however, it is still undefined behavior that can result in
+ strange errors due to compiler optimizations).
+
+- ``non-overlapping-ranges`` -- for functions that take several ranges as
+ arguments, checks that the given ranges do not overlap.
+
+- ``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.
+
+- ``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). 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.
+
+- ``compatible-allocator`` -- checks any operations that exchange nodes between
+ containers to make sure the containers have compatible allocators.
+
+- ``argument-within-domain`` -- checks that the given argument is within the
+ domain of valid arguments for the function. Violating this typically produces
+ an incorrect result (e.g. the clamp algorithm returns the original value
+ without clamping it due to incorrect functors) or puts an object into an
+ invalid state (e.g. a string view where only a subset of elements is possible
+ to access). This category is for assertions violating which doesn't cause any
+ immediate issues in the library -- whatever the consequences are, they will
+ happen in the user code.
+
+- ``pedantic`` -- checks prerequisites that are imposed by the Standard, but
+ violating which happens to be benign in our implementation.
+
+- ``semantic-requirement`` -- checks that the given argument satisfies the
+ semantic requirements imposed by the Standard. Typically, there is no simple
+ way to completely prove that a semantic requirement is satisfied; thus, this
+ would often be a heuristic check and it might be quite expensive.
+
+- ``internal`` -- checks that internal invariants of the library hold. These
+ assertions don't depend on user input.
+
+- ``uncategorized`` -- for assertions that haven't been properly classified yet.
+ This is an escape hatch used for some existing assertions in the library; all
+ new code should have its assertions properly classified.
+
+Mapping between the hardening modes and the assertion categories
+================================================================
+
+.. list-table::
+ :header-rows: 1
+ :widths: auto
+
+ * - Category name
+ - ``fast``
+ - ``extensive``
+ - ``debug``
+ * - ``valid-element-access``
+ - ✅
+ - ✅
+ - ✅
+ * - ``valid-input-range``
+ - ✅
+ - ✅
+ - ✅
+ * - ``non-null``
+ - ❌
+ - ✅
+ - ✅
+ * - ``non-overlapping-ranges``
+ - ❌
+ - ✅
+ - ✅
+ * - ``valid-deallocation``
+ - ❌
+ - ✅
+ - ✅
+ * - ``valid-external-api-call``
+ - ❌
+ - ✅
+ - ✅
+ * - ``compatible-allocator``
+ - ❌
+ - ✅
+ - ✅
+ * - ``argument-within-domain``
+ - ❌
+ - ✅
+ - ✅
+ * - ``pedantic``
+ - ❌
+ - ✅
+ - ✅
+ * - ``semantic-requirement``
+ - ❌
+ - ❌
+ - ✅
+ * - ``internal``
+ - ❌
+ - ❌
+ - ✅
+ * - ``uncategorized``
+ - ❌
+ - ✅
+ - ✅
+
+.. note::
+
+ At the moment, each subsequent hardening mode is a strict superset of the
+ previous one (in other words, each subsequent mode only enables additional
+ assertion categories without disabling any), but this won't necessarily be
+ true for any hardening modes that might potentially be added in the future.
+
+Hardening assertion failure
+===========================
+
+In production modes (``fast`` and ``extensive``), a hardening assertion failure
+immediately traps the program. This is the safest approach that also minimizes
+the code size penalty as the failure handler maps to a single instruction. The
+downside is that the failure provides no additional details other than the stack
+trace (which might also be affected by optimizations).
+
+TODO(hardening): describe ``__builtin_verbose_trap`` once we can use it.
+
+In the ``debug`` mode, an assertion failure terminates the program in an
+unspecified manner and also outputs the associated error message to the error
+output. This is less secure and increases the size of the binary (among other
+things, to store the error message strings) but makes the failure easier to
+debug. It also allows us to test the error messages in our test suite.
+
+.. _override-assertion-handler:
+
+Overriding the assertion failure handler
+----------------------------------------
+
+Vendors can override the default termination handler mechanism by following
+these steps:
+
+- create a header file that provides a definition of a macro called
+ ``_LIBCPP_ASSERTION_HANDLER``. The macro will be invoked when a hardening
+ assertion fails, with a single parameter containing a null-terminated string
+ with the error message.
+- when configuring the library, provide the path to custom header (relative to
+ the root of the repository) via the CMake variable
+ ``LIBCXX_ASSERTION_HANDLER_FILE``.
+
+ABI
+===
+
+Setting a hardening mode does **not** affect the ABI. Each mode uses the subset
+of checks available in the current ABI configuration which is determined by the
+platform.
+
+It is important to stress that whether a particular check is enabled depends on
+the combination of the selected hardening mode and the hardening-related ABI
+options. Some checks require changing the ABI from the "default" to store
+additional information in the library classes -- e.g. checking whether an
+iterator is valid upon dereference generally requires storing data about bounds
+inside the iterator object. Using ``std::span`` as an example, setting the
+hardening mode to ``fast`` will always enable the ``valid-element-access``
+checks when accessing elements via a ``span`` object, but whether dereferencing
+a ``span`` iterator does the equivalent check depends on the ABI configuration.
+
+ABI options
+-----------
+
+Vendors can use the following ABI options to enable additional hardening checks:
+
+- ``_LIBCPP_ABI_BOUNDED_ITERATORS`` -- changes the iterator type of select
+ containers (see below) to a bounded iterator that keeps track of whether it's
+ within the bounds of the original container and asserts it on every
+ dereference.
+
+ ABI impact: changes the iterator type of the relevant containers.
+
+ Supported containers:
+ - ``span``;
+ - ``string_view``.
+
+ABI tags
+--------
+
+We use ABI tags to allow translation units built with different hardening modes
+to interact with each other without causing ODR violations. Knowing how
+hardening modes are encoded into the ABI tags might be useful to examine
+a binary and determine whether it was built with hardening enabled.
+
+.. warning::
+ We don't commit to the ABI tags being stable between different releases of
+ libc++. The following describes the state of the latest release and is for
+ informational purposes only.
+
+The first character of an ABI tag encodes the hardening mode:
+
+- ``f`` -- [f]ast mode;
+- ``s`` -- extensive ("[s]afe") mode;
+- ``d`` -- [d]ebug mode;
+- ``n`` -- [n]one mode.
+
+Hardened containers status
+==========================
+
+.. list-table::
+ :header-rows: 1
+ :widths: auto
+
+ * - Name
+ - Member functions
+ - Iterators (ABI-dependent)
+ * - ``span``
+ - ✅
+ - ✅
+ * - ``string_view``
+ - ✅
+ - ✅
+ * - ``array``
+ - ✅
+ - ❌
+ * - ``vector``
+ - ✅
+ - ❌
+ * - ``string``
+ - ✅
+ - ❌
+ * - ``list``
+ - ✅
+ - ❌
+ * - ``forward_list``
+ - ❌
+ - ❌
+ * - ``deque``
+ - ✅
+ - ❌
+ * - ``mdspan``
+ - ✅
+ - ❌
+ * - ``optional``
+ - ✅
+ - N/A
+
+TODO(hardening): make this table exhaustive.
+
+Testing
+=======
+
+Each hardening assertion should be tested using death tests (via the
+``TEST_LIBCPP_ASSERT_FAILURE`` macro). Use the ``libcpp-hardening-mode`` Lit
+feature to make sure the assertion is enabled in (and only in) the intended
+modes. Note that error messages are only tested (matched) if the ``debug``
+hardening mode is used.
+
+Further reading
+===============
+
+- ``_Hardening RFC <https://discourse.llvm.org/t/rfc-hardening-in-libc/73925>``:
+ contains some of the design rationale.
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index fcd630e09b449..4f7b9b362e5e6 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -40,7 +40,7 @@ and C++26 features.
New hardened modes for the library have been added, replacing the legacy debug mode that was
removed in the LLVM 17 release. Unlike the legacy debug mode, some of these hardening modes are
-also intended to be used in production. See :ref:`hardening-modes` for more details.
+also intended to be used in production. See :ref:`hardening` for more details.
Work on the ranges support has progressed. See
:ref:`ranges-status` for the current status.
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 104a244cc82cc..7dab5a2b6dcaa 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -202,8 +202,7 @@
//
// Supported containers:
// - `span`;
-// - `string_view`;
-// - `array`.
+// - `string_view`.
// #define _LIBCPP_ABI_BOUNDED_ITERATORS
// } ABI
|
There was a problem hiding this comment.
As usual, @var-const , your documentation is amazing. I hope that these nits are helpful -- it's the only way that I can help!
libcxx/docs/Hardening.rst
Outdated
| including heuristic checks that might have significant performance overhead as | ||
| well as internal library assertions. This mode should be used in | ||
| non-production environments (such as test suites, CI, local development). We | ||
| don’t commit to a particular level of performance in this mode and it’s *not* |
There was a problem hiding this comment.
I would start the text that it's not intended for production usage, that way the reader can skip this directly when looking for options suitable for production usage.
There was a problem hiding this comment.
I find it a little hard to rephrase without breaking the flow. I'd prefer mode descriptions to follow the consistent pattern where each paragraph begins with describing the enabled checks, followed by some additional remarks. I think the name debug already strongly implies it's not suitable for production use, and the paragraphs are relatively short, so I don't think it should cause any significant confusion to the reader.
libcxx/docs/Hardening.rst
Outdated
| enabled in the ABI. Types like ``optional`` and ``function`` are considered | ||
| one-element containers for the purposes of this check. |
There was a problem hiding this comment.
This is a bit unclear. Is an optional<int> foo considered a container with one or zero elements? I would expect zero in this case.
There was a problem hiding this comment.
I guess the intended meaning is more like "at most one-element container" or "up to one element container". Would you prefer to make it explicit in the text?
| - the sentinel is reachable from the begin iterator; | ||
| - TODO(hardening): both iterators refer to the same container. | ||
|
|
||
| ("input" here refers to "an input given to an algorithm", not to an iterator |
There was a problem hiding this comment.
Should we consider renaming valid-input-range to valid-range-argument to avoid the possible confusion?
There was a problem hiding this comment.
+1 -- I'll prepare a follow-up.
| - ✅ | ||
| - ❌ | ||
| * - ``optional`` | ||
| - ✅ |
There was a problem hiding this comment.
I miss
map,set, and their multi and unordered parts. (we don't have the flat ones)valarraybitset(this has no iterators, but has an index operation.- the container adapters,
stack,queueandpriority_queue functionwas listed above- what about
variant,any, andexpected?
There was a problem hiding this comment.
Thanks! Would you be okay if I do this in an (immediate) follow-up?
|
@hawkinsw Thank you for your suggestions! I have implemented most of them, please let me know if you feel strongly about any of the remaining ones. |
Thank you for doing this great work -- your documentation is always so thorough. I resolved all but one -- feel free to resolve that after you read the minor comment I wrote. Thank you again! |
|
@mordante PTAL whenever you have the time. I think I have addressed all of your current comments (please unresolve anything that you feel wasn't fully addressed). |
| well as internal library assertions. This mode should be used in | ||
| non-production environments (such as test suites, CI, or local development). | ||
| We don’t commit to a particular level of performance in this mode and it’s | ||
| *not* intended to be used in production. |
There was a problem hiding this comment.
Should we add:
However, we do not change the complexity of algorithms since satisfying the complexity of an algorithm is necessary for Standards conformance.
Or, if we don't want to commit to that, then we should mention the contrary. Either way, it seems like a nice place to document this design point.
There was a problem hiding this comment.
I strongly suspect that not changing the complexity is the right approach, but I'm still very slightly hesitant to commit to it at this point, given that our focus has been mainly on the fast mode and not on the debug mode. Do you think we could simply write that we're not sure although leaning towards not changing? Or should we only document decisions that we're pretty sure about, to avoid confusing our users?
There was a problem hiding this comment.
I don't think we should document something like "we think we're going to do X but we're not certain". That doesn't add any clarity as far as users are concerned. If we're too unsure about what we want to do, I would recommend just not saying anything (i.e. status quo).
There was a problem hiding this comment.
I think this is absolutely worth documenting, but I'd prefer to do it at a later point after we spend more time restoring feature parity with the old debug mode and become more confident that this is the right approach.
libcxx/docs/Hardening.rst
Outdated
| the root of the repository) via the CMake variable | ||
| ``LIBCXX_ASSERTION_HANDLER_FILE``. | ||
|
|
||
| There is no existing mechanism for users to override the termination handler. |
There was a problem hiding this comment.
| There is no existing mechanism for users to override the termination handler. | |
| There is no existing mechanism for users to override the termination handler, however a vendor who wishes to provide this capability is free to do so, such as by declaring the assertion handler as an overridable function. |
This would also be a good place to say something like:
This is done because the ability to override the assertion handler other than at configure-time creates a code size tradeoff that may not be suitable in all cases. Instead, we let vendors decide what's right on their platform for their users.
There was a problem hiding this comment.
Added (with some rephrasing).
| ABI tags | ||
| -------- | ||
|
|
||
| We use ABI tags to allow translation units built with different hardening modes | ||
| to interact with each other without causing ODR violations. Knowing how | ||
| hardening modes are encoded into the ABI tags might be useful to examine | ||
| a binary and determine whether it was built with hardening enabled. | ||
|
|
||
| .. warning:: | ||
| We don't commit to the encoding scheme used by the ABI tags being stable | ||
| between different releases of libc++. The tags themselves are never stable, by | ||
| design -- new releases increase the version number. The following describes | ||
| the state of the latest release and is for informational purposes only. | ||
|
|
||
| The first character of an ABI tag encodes the hardening mode: | ||
|
|
||
| - ``f`` -- [f]ast mode; | ||
| - ``s`` -- extensive ("[s]afe") mode; | ||
| - ``d`` -- [d]ebug mode; | ||
| - ``n`` -- [n]one mode. |
There was a problem hiding this comment.
I am worried that this is going to become stale very quickly.
I do agree it is useful for users to be able to know the configuration they're looking at. I'm conflicted between keeping this section and removing it.
There was a problem hiding this comment.
I do share this concern. I tried to stress that in the warning note, do you think emphasizing this even more would help?
libcxx/docs/Hardening.rst
Outdated
| - ✅ | ||
| - N/A | ||
|
|
||
| TODO(hardening): make this table exhaustive. |
There was a problem hiding this comment.
IMO we should add the containers we know we're missing (Mark's comment above) and resolve this TODO immediately, in this patch. Unless that change ends up being large and we want to decouple it?
There was a problem hiding this comment.
Done.
Some of these are a little tricky:
- We do have some hardening in
valarray, I can't immediately say if it covers everything or not. Went with a conservative "partial"; - I'm not sure how much hardening we need in tree-based containers since they don't provide direct access to memory. Once again, I conservatively state that they are not hardened (in the sense that there are no interesting hardening checks there)
- re.
anyandvariant-- is it possible to use those incorrectly without getting a guaranteed exception?
| with the error message. | ||
| - when configuring the library, provide the path to custom header (relative to | ||
| the root of the repository) via the CMake variable | ||
| ``LIBCXX_ASSERTION_HANDLER_FILE``. |
There was a problem hiding this comment.
Perhaps this is the right place to mention that the assertion handler gets included everywhere, so there must not be dependencies on other parts of the library in that header.
There was a problem hiding this comment.
Done. Should we mention that __config is an exception? (Not sure if we're okay with vendors including it since it's a private header)
There was a problem hiding this comment.
We could say should not include anything non trivial to keep things somewhat vague. I'm also fine with mentioning __config directly, it just seems a bit too restrictive to prevent including anything else (since several headers don't have a dependency on the handler). Basically I think the intent we should go for is to warn vendors about the potential for circular deps and then let them figure out something that works.
There was a problem hiding this comment.
Went with should not include anything non trivial (it seems easy enough to use the default header as an example and notice that it does include __config).
var-const
force-pushed
the
varconst/hardening-docs
branch
from
1b21cc2 to
b211dec
Compare
| well as internal library assertions. This mode should be used in | ||
| non-production environments (such as test suites, CI, or local development). | ||
| We don’t commit to a particular level of performance in this mode and it’s | ||
| *not* intended to be used in production. |
There was a problem hiding this comment.
I don't think we should document something like "we think we're going to do X but we're not certain". That doesn't add any clarity as far as users are concerned. If we're too unsure about what we want to do, I would recommend just not saying anything (i.e. status quo).
| output. This is less secure and increases the size of the binary (among other | ||
| things, it has to store the error message strings) but makes the failure easier |
There was a problem hiding this comment.
I think your current formulation is acceptable. I initially had trouble parsing it cause I missed the beginning of the ( but I think status quo is fine.
| with the error message. | ||
| - when configuring the library, provide the path to custom header (relative to | ||
| the root of the repository) via the CMake variable | ||
| ``LIBCXX_ASSERTION_HANDLER_FILE``. |
There was a problem hiding this comment.
We could say should not include anything non trivial to keep things somewhat vague. I'm also fine with mentioning __config directly, it just seems a bit too restrictive to prevent including anything else (since several headers don't have a dependency on the handler). Basically I think the intent we should go for is to warn vendors about the potential for circular deps and then let them figure out something that works.
No description provided.