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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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);
----------------
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?


================
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");
----------------
ldionne wrote:
> Separate from the `ASSERT_VALID_RANGE` question, do we want to introduce helper functions to help checking whether a range is valid?
Yes, this is something I'd really love to do. I'd do this in a separate patch, though, if that's okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155873