[libcxx-commits] [PATCH] D155349: [libc++][hardening] Categorize most assertions inside the container classes.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 2 inline comments as done.
var-const added inline comments.


================
Comment at: libcxx/include/__config:252
+//
+// - `_LIBCPP_ASSERT_VALID_ALLOCATOR` -- checks any operations that merge or swap containers to make sure the containers
+//   have compatible allocators.
----------------
Mordante wrote:
> I expect in the future there might be other tests on allocators. So I think using a longer name leaves more space for future naming.
How about `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR`?


================
Comment at: libcxx/include/__config:255
+//
+// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
+//   user input.
----------------
Mordante wrote:
> Likewise leaving room for other internal hardining options.
In this case, at least for now, I'd prefer to use a broader category. These are the assertions that most likely will only be used in the debug mode, so I don't think that being more fine-grained here would make a visible difference, while it adds an additional (however small) burden when writing new code to pick the right category.


================
Comment at: libcxx/include/list:1666
 {
-    _LIBCPP_ASSERT_UNCATEGORIZED(this != _VSTD::addressof(__c),
-                                 "list::splice(iterator, list) called with this == &list");
+    _LIBCPP_ASSERT_VALID_CONTAINER_ACCESS(this != _VSTD::addressof(__c),
+                                          "list::splice(iterator, list) called with this == &list");
----------------
ldionne wrote:
> Maybe this should be `_LIBCPP_ASSERT_VALID_INPUT_RANGE`?
Changed. I think this is closest in spirit to checking that e.g. the two ranges given to an algorithm like `std::copy` aren't overlapping. Since our current idea is to keep the number of categories within some reasonable bounds, merging this into the `valid-input-range` category makes sense.


================
Comment at: libcxx/include/span:246
+      _LIBCPP_ASSERT_VALID_INPUT_RANGE(__dist >= 0, "invalid range in span's constructor (iterator, sentinel)");
       _LIBCPP_ASSERT_UNCATEGORIZED(
           __dist == _Extent, "invalid range in span's constructor (iterator, sentinel): last - first != extent");
----------------
ldionne wrote:
> I think this should be `_LIBCPP_ASSERT_VALID_INPUT_RANGE` as well. Technically, it isn't really a "valid-input-range" type of check, however it's necessary to enable it in the hardened mode (otherwise `operator[]` checking can be defeated). Or maybe it should be `VALID_CONTAINER_ACCESS` since that is what this one protects against.
Done. Decided to go with `valid-container-access` as it seems to be closer in intention like you mentioned.


================
Comment at: libcxx/include/span:264
     constexpr explicit span(_Range&& __r) : __data_{ranges::data(__r)} {
       _LIBCPP_ASSERT_UNCATEGORIZED(ranges::size(__r) == _Extent, "size mismatch in span's constructor (range)");
     }
----------------
ldionne wrote:
> The same comment applies here and below, those need to be enabled in the hardened mode.
Done throughout `span`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155349