The safe mode is in-between the hardened and the debug modes, extending
the checks contained in the hardened mode with certain checks that are
relatively cheap and prevent common sources of errors but aren't
security-critical. Thus, the safe mode trades off some performance for
a wider set of checks, but unlike the debug mode, it can still be used
in production.
Details
- Reviewers
jdoerfert ldionne - Group Reviewers
Restricted Project - Commits
- rGb85e1862c119: [libc++][hardening] Add back the safe mode.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/CMakeLists.txt | ||
---|---|---|
51 | We need tests. | |
53 | @thakis We brainstormed on some names here: _LIBCPP_ENABLE_HARDENED_PLUS_MODE _LIBCPP_ENABLE_EXTENDED_HARDENED_MODE _LIBCPP_ENABLE_STRONG_HARDENED_MODE _LIBCPP_ENABLE_STRICT_MODE _LIBCPP_ENABLE_PARANOID_MODE _LIBCPP_ENABLE_FORTIFIED_MODE _LIBCPP_ENABLE_SAFE_MODE Do you have any thoughts? Our thoughts so far: HARDENED_PLUS, EXTENDED_HARDENED, STRONG_HARDENED // those are kind of heavyweight names PARANOID // has negative connotation and doesn't make it clear whether it is stronger than DEBUG STRICT // could be confused with the notion of not having non-standard extensions FORTIFIED // not clear whether it is stronger than HARDENED or not SAFE // our current preference In fact, before LLVM 17 we had something called the SAFE mode, and I think what we discovered with Chromium's use case is that it still had its place. Hence, I think what we should do is call this the SAFE mode, backport this change, and rework the way we announced our 17 release notes not to say that we "replaced' the safe mode, but instead that we added new modes and that we changed how the safe mode is enabled. This is IMO a superior design and a superior way of rolling it out based on our experience so far. | |
libcxx/include/__algorithm/three_way_comp_ref_type.h | ||
53 ↗ | (On Diff #553406) | I think this should be a separate (almost NFC) patch. There might be more of those, we should fix all. |
libcxx/include/__algorithm/three_way_comp_ref_type.h | ||
---|---|---|
53 ↗ | (On Diff #553406) |
libcxx/docs/Hardening.rst | ||
---|---|---|
58 | ||
libcxx/docs/ReleaseNotes/17.rst | ||
109 | We should replace all these references to libcxx/docs/Hardening.rst by an internal link to the rst page so that people can actually click on it. | |
111–117 | ||
134–135 | ||
libcxx/include/__config | ||
281–286 | No strong preference, but we could also do #if HARDENED_MODE + SAFE_MODE + DEBUG_MODE > 1. | |
libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp | ||
11–13 | I don't really understand this REQUIRES anymore. Can you explain? |
libcxx/cmake/caches/Generic-safe-mode.cmake | ||
---|---|---|
2 | I really would like to enable modules in this build too. Maybe it would be better to do in a followup when we want to backport this. | |
libcxx/utils/ci/buildkite-pipeline.yml | ||
508–509 | ||
libcxx/utils/libcxx/test/params.py | ||
302–304 | I start to feel slightly uncomfortable with these names. To me they are not very descriptive and they are now 4 options. I don't directly have better suggestions, but I think we should spend a bit of time on this. For example, which is more expensive "safe" or "hardened"? |
libcxx/CMakeLists.txt | ||
---|---|---|
53 | @Mordante Those are the other names we considered. | |
libcxx/utils/libcxx/test/params.py | ||
302–304 | I agree, this is definitely not perfect. This is user facing too, so we need to find something good. We thought that safe was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than hardened, but maybe it isn't. I'll CC you on the comment above that discusses naming. |
I believe this 3-level thing is in general a good idea, and in principle the words "hardened < safe < debug" fell ok-ish to me in terms of expectations how much gets checked.
However, in light of a 3-level mode I may want to rethink for example the asserts in mdspan, and tailor better what is enabled in hardened, vs safe mode. I think the current choices were good in a binary world, in this more nuanced approach I am not sure.
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
302–304 |
Based on this information and Discord I think we should go with safe. I still think it's not great, but it's consistent with what we did in the past. |
Thanks a lot for your comments! Re. mdspan -- IIRC, we classified most of the assertions in mdspan as valid-element-access, which would enable them in all modes. The only exceptions are:
- layout_left/right::mapping checks -- IMO those should be enabled in the safe mode;
- mdspan: size() is not representable as size_type -- I also think this should be on in the safe mode.
(FWIW, my current informal criteria for the safe mode is "any check as long as it's for UB triggered by user input and not too expensive". With that mindset, none of the checks I remember in mdspan strike me as debug-only -- i.e., all of them make sense in the safe mode as well)
libcxx/CMakeLists.txt | ||
---|---|---|
53 | @Mordante Thank you for the feedback! We spent quite a bit of time on this but unfortunately I don't have a "slam dunk" suggestion. FWIW, I'll outline my reasoning:
| |
libcxx/docs/ReleaseNotes/17.rst | ||
134–135 | Applied (with a few tweaks, please let me know if they look reasonable to you). | |
libcxx/include/__config | ||
281–286 | Thanks! I think it's better (I don't really expect another mode at this point, but if that ever happens, the new form of this check is much easier to modify). | |
libcxx/utils/libcxx/test/params.py | ||
302–304 | (I'm marking these comments as "Done" just so that we can keep this conversation in one thread) |
libcxx/cmake/caches/Generic-safe-mode.cmake | ||
---|---|---|
2 | Thanks! I'll do it in a follow-up like you suggest. | |
libcxx/include/__config | ||
281–286 | Update: unfortunately, with this approach the error is also triggered when setting one of the macros to a bad value (e.g. 2). Reverted to the previous state. | |
libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp | ||
11–13 | If another hardening mode is enabled (e.g. the safe mode), the assertions will still fire -- basically, we would end up in a situation where _LIBCPP_ENABLE_DEBUG_MODE is 0 but _LIBCPP_ENABLE_SAFE_MODE is 1. (I changed the assertion type from "uncategorized" to "valid-element-access", btw -- thanks for calling out this test!) |
libcxx/cmake/caches/Generic-safe-mode.cmake | ||
---|---|---|
2 |
@thakis It would be awesome if you could take a look at this patch and see if naming and the approach make sense for Chromium's use case. Thanks!
This looks perfect for us, thanks!
libcxx/CMakeLists.txt | ||
---|---|---|
53 | As long as it's documented, any of those work for me. Naming tends to be a bikeshed. Here's some more ideas (for the cmake arg), but go with what you like best: some, most, debug i.e. maybe renaming the "hardened" level could make the progression clearer. If you want to keep "hardened" as base level, maybe "thorough" or "conservative" are still contenders. But as I said, I don't mind "safe". | |
libcxx/include/__config | ||
313 | TODO when? After a security review of these asserts? Something else? Do you have a rough time frame when this might happen? |
libcxx/docs/ReleaseNotes/17.rst | ||
---|---|---|
89–90 | I would prefer removing this release note entirely. It is misleading, I don't know why I added it in the first place. OOB checks in iterators for string_view require an ABI flag, which needs to be set by vendors. This is not interesting to point out in our release notes IMO. | |
libcxx/include/__config | ||
225–226 | ||
313 | IMO this TODO is not necessary. We know we want to remove _LIBCPP_ASSERT_UNCATEGORIZED anyway so it would become moot. | |
libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp | ||
13 | ||
libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp | ||
1 | For another patch: we need to add tests that ensure that we can enable/disable the various hardening modes on a per-TU basis even if the default configured mode is another mode. For example, configure the library w/ the hardened mode by default, but enable the safe or debug mode inside a single TU via the user macros. Or the other way around: enable safe mode by default via vendor but enable the hardened mode via users: the result should be that the safe mode set-by-default is disabled and the hardened mode selected by the user is enabled. Potential way to achieve this? #if _LIBCPP_ENABLE_HARDENED_MODE # define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENED #elif _LIBCPP_ENABLE_SAFE_MODE # define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_SAFE #elif _LIBCPP_ENABLE_DEBUG_MODE # define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_DEBUG #endif #define _LIBCPP_UNCHECKED 31 #define _LIBCPP_HARDENED 32 #define _LIBCPP_SAFE 33 #define _LIBCPP_DEBUG 34 #ifndef _LIBCPP_SELECT_HARDENING_MODE # define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENING_MODE_DEFAULT /* from __config_site */ #endif _LIBCPP_SELECT_HARDENING_MODE would be internal-only, unless we want to migrate our users again in the next release :/ And if we want to make _LIBCPP_SELECT_HARDENING_MODE user-visible without migrating our users, we could probably do something like: #ifndef _LIBCPP_SELECT_HARDENING_MODE # if _LIBCPP_ENABLE_HARDENED_MODE # define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENED # elif [...] # endif #endif | |
9 | This comment is misleading otherwise. | |
11–13 | IMO this should be REQUIRES: libcpp-hardening-mode=debug since the test is basically meaningless in unchecked mode. | |
libcxx/test/libcxx/assertions/modes/debug_mode_enabled_in_tu.pass.cpp | ||
23 | UNCATEGORIZED? |
libcxx/docs/ReleaseNotes/17.rst | ||
---|---|---|
89–90 | Addressed in https://reviews.llvm.org/D159171 |
libcxx/docs/ReleaseNotes/17.rst | ||
---|---|---|
82 | Note that this patch needs to be rebased on top of another patch that brings the release notes between LLVM 17 and main back into sync. |
libcxx/docs/ReleaseNotes/17.rst | ||
---|---|---|
82 | Rebased on top of https://reviews.llvm.org/D159454.. |
We need tests.