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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D155349#4503312 <https://reviews.llvm.org/D155349#4503312>, @Mordante wrote:

> Once we expose these names to users it will be a lot harder to change them.

Just to clarify, the intent is that those names are private. Users should only toggle the high-level options `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_DEBUG_MODE`. We specifically didn't want to lock ourselves into these categories if we wanted to change them later on.

This looks pretty good to me, with a few comments. The categories make sense to me, and I think we'll learn more about how we want to categorize stuff going forward (but since this isn't public, we can adjust as we learn more).



================
Comment at: libcxx/include/__config:236
 
-// Available checks:
+// Available assertion categories:
 
----------------
You should make it clear that this is "internal documentation". Maybe something like:

```
Inside the library, assertions are categorized so they can be cherry-picked based on the hardening mode. Those are internal-only macros, users should stick to the high-level modes above.
```


================
Comment at: libcxx/include/__config:249
+//
+// - `_LIBCPP_ASSERT_VALID_ITERATOR_ACCESS` -- checks that iterators being dereferenced are valid. Bounded iterators
+//   must be enabled in the ABI for this check to work (otherwise it will be a no-op).
----------------
As discussed: I think it would also make sense to merge this with `VALID_CONTAINER_ACCESS` since those are effectively the same.


================
Comment at: libcxx/include/__config:260
+
+// TODO(hardening): add documentation for any new checks here.
 
----------------
I think this TODO can be removed now, since it's not an actionable thing anymore.


================
Comment at: libcxx/include/__config:288-289
+#    define _LIBCPP_ASSERT_VALID_ITERATOR_ACCESS(expression, message)  _LIBCPP_ASSERT(expression, message)
+// TODO(hardening): Don't enable uncategorized assertions in the hardened mode.
+#    define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)          _LIBCPP_ASSERT(expression, message)
+// Disabled checks.
----------------
I think you can probably just do it in this patch? Or otherwise let's do in a follow-up right after this patch.


================
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");
----------------
Maybe this should be `_LIBCPP_ASSERT_VALID_INPUT_RANGE`?


================
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");
----------------
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.


================
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)");
     }
----------------
The same comment applies here and below, those need to be enabled in the hardened mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155349