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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I really like this hardening work. I think it would be good to have bit more discussion and tweaking after LLVM 17 has branched. I would love it when a security expert can have a look at it. I expect they have a better view on what root causes for security issues are.



================
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()");
----------------
ldionne wrote:
> 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.
+1


================
Comment at: libcxx/include/__tree:171
 {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__x != nullptr, "Root node shouldn't be null");
+    _LIBCPP_ASSERT_INTERNAL(__x != nullptr, "Root node shouldn't be null");
     while (__x->__left_ != nullptr)
----------------
As mentioned in D155906 I really would like to have these kind of cheap internal sanity checks enabled in `_LIBCPP_ENABLE_HARDENED_MODE`. They are not user errors, but they still cause potential issues for users, including users in safety-critial places. When there's not enough time to get that done before LLVM 17, I'm happy to defer that to LLVM 18.


================
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);
----------------
var-const wrote:
> ldionne wrote:
> > 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.
> It's a great question and a topic for discussion, IMO. I think reviewing this with a security expert would be great.
> 
> I'd prefer to avoid blocking the patch on this, though, given that the branch cutoff date is coming up, and a large part of the patch seems to be more straightforward than the questions related to non-null checks. I'd remove the `NON_NULL` part from this patch and do it in a follow-up instead, does that sound reasonable to you?
+1 for having security expert having a look on this. I would like to have this check in hardened mode. This is check to check and users of libc++ may do the "wrong thing". But not a blocker for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155873