[libcxx-commits] [PATCH] D155873: [libc++][hardening] Categorize more assertions.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__config:279
 // clang-format off
 #  if _LIBCPP_ENABLE_HARDENED_MODE
 
----------------
It would be great to rebase this on top of D155866.


================
Comment at: libcxx/include/__config:287
 // Disabled checks.
+#    define _LIBCPP_ASSERT_NON_NULL(expression, message)               _LIBCPP_ASSUME(expression)
 #    define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)   _LIBCPP_ASSUME(expression)
----------------
Here we could add a short comment rationalizing the choice: `Nullptr conditions are generally meant to protect against dereferencing a null pointer, which generally results in a crash`.


================
Comment at: libcxx/include/__hash_table:1112
     {
-        _LIBCPP_ASSERT_UNCATEGORIZED(__n < bucket_count(),
+        _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n < bucket_count(),
             "unordered container::begin(n) called with n >= bucket_count()");
----------------
It would be nice to make sure that all the non-internal assertions we add are tested. In some cases this will mean removing `XFAIL` on an existing test, in other cases we'll need to add some tests but it should be easy.


================
Comment at: libcxx/include/__string/char_traits.h:145
         if (!__libcpp_is_constant_evaluated()) {
-            _LIBCPP_ASSERT_UNCATEGORIZED(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+            _LIBCPP_ASSERT_VALID_INPUT_RANGE(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
         }
----------------
Maybe `_LIBCPP_ASSERT_NON_OVERLAPPING_RANGES`? Both ranges can technically be valid ranges yet still be overlapping. Also, the result of the algo will be incorrect if the ranges overlap but I don't think this can directly cause bad stuff to happen, so I don't think we want this in the hardened mode. Using another category would allow excluding it.


================
Comment at: libcxx/include/string:956
       : __r_(__default_init_tag(), __default_init_tag()) {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__n == 0 || __s != nullptr, "basic_string(const char*, n) detected nullptr");
+    _LIBCPP_ASSERT_NON_NULL(__n == 0 || __s != nullptr, "basic_string(const char*, n) detected nullptr");
     __init(__s, __n);
----------------
I think a lot of these `NON_NULL` checks are actually `VALID_RANGE` checks. http://eel.is/c++draft/iterators#iterator.requirements.general-11

Here we seem to be using `__s != nullptr` as a way to say `__s` is dereferenceable. Do we want to move these checks to `ASSERT_VALID_RANGE`? Do we want these checks in hardened mode? I'm not sure, I think we should probably review this with a security specialist.


================
Comment at: libcxx/include/string:963
       : __r_(__default_init_tag(), __a) {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__n == 0 || __s != nullptr,
-                                 "basic_string(const char*, n, allocator) detected nullptr");
+    _LIBCPP_ASSERT_NON_NULL(__n == 0 || __s != nullptr,
+                            "basic_string(const char*, n, allocator) detected nullptr");
----------------
Separate from the `ASSERT_VALID_RANGE` question, do we want to introduce helper functions to help checking whether a range is valid?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155873