[libcxx-commits] [PATCH] D158823: [libc++][hardening] Add back the safe mode.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes/17.rst:90
 
-- ``std::string_view`` now provides iterators that check for out-of-bounds accesses when the safe
-  libc++ mode is enabled.
+- ``std::string_view`` now provides iterators that check for out-of-bounds accesses when hardening is enabled.
 
----------------
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.


================
Comment at: libcxx/include/__config:231-232
+// Enables the safe mode which extends the hardened mode with checks that are relatively cheap and prevent common types
+// of errors but are not security-critical.
+// `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_DEBUG_MODE`.
+//
----------------



================
Comment at: libcxx/include/__config:319
+#    define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)     _LIBCPP_ASSERT(expression, message)
+// TODO(hardening): don't enable uncategorized assertions in the safe mode.
+#    define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)            _LIBCPP_ASSERT(expression, message)
----------------
thakis wrote:
> TODO when? After a security review of these asserts? Something else? Do you have a rough time frame when this might happen?
IMO this TODO is not necessary. We know we want to remove `_LIBCPP_ASSERT_UNCATEGORIZED` anyway so it would become moot.


================
Comment at: libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp:13
 // UNSUPPORTED: c++03, c++11, c++14, c++17
-// UNSUPPORTED: !libcpp-hardening-mode=debug
+// UNSUPPORTED: !libcpp-hardening-mode=safe && !libcpp-hardening-mode=debug
 // XFAIL: availability-verbose_abort-missing
----------------



================
Comment at: 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
```


================
Comment at: libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp:9
 
 // This test ensures that we can disable the debug mode on a per-TU basis regardless of how the library was built.
 
----------------
This comment is misleading otherwise.


================
Comment at: libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp:11
 
-// UNSUPPORTED: libcpp-hardening-mode=hardened
+// REQUIRES: libcpp-hardening-mode=unchecked || libcpp-hardening-mode=debug
 // ADDITIONAL_COMPILE_FLAGS: -Wno-macro-redefined -D_LIBCPP_ENABLE_DEBUG_MODE=0
----------------
var-const wrote:
> ldionne wrote:
> > I don't really understand this `REQUIRES` anymore. Can you explain?
> 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!)
IMO this should be `REQUIRES: libcpp-hardening-mode=debug` since the test is basically meaningless in unchecked mode.


================
Comment at: libcxx/test/libcxx/assertions/modes/debug_mode_enabled_in_tu.pass.cpp:23
 int main(int, char**) {
   _LIBCPP_ASSERT_UNCATEGORIZED(true, "Should not fire");
   TEST_LIBCPP_ASSERT_FAILURE([] {
----------------
`UNCATEGORIZED`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158823/new/

https://reviews.llvm.org/D158823