This is an archive of the discontinued LLVM Phabricator instance.

[libc++][hardening] Categorize most assertions inside the container classes.
ClosedPublic

Authored by var-const on Jul 14 2023, 5:01 PM.

Details

Summary

This introduces:

  • _LIBCPP_ASSERT_VALID_INPUT_RANGE;
  • _LIBCPP_ASSERT_VALID_CONTAINER_ACCESS;
  • _LIBCPP_ASSERT_VALID_ITERATOR_ACCESS;
  • _LIBCPP_ASSERT_VALID_ALLOCATOR;
  • _LIBCPP_ASSERT_INTERNAL.

Diff Detail

Event Timeline

var-const created this revision.Jul 14 2023, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 5:01 PM
var-const requested review of this revision.Jul 14 2023, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 5:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this! In general LGMT. Some remarks regarding the naming; I know it seems a bit like bike shedding, but IMO we should have good names. Once we expose these names to users it will be a lot harder to change them. I feel some names are quite "short" and make it hard to use similar tests in the future with a consistent name.

libcxx/include/__config
237

This misses iterator and sentinel.

245

I see the point of calling everything a container and making them a one element container. I wonder whether the name _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS would be better.

250

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.

253

Likewise leaving room for other internal hardining options.

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).

libcxx/include/__config
235–236

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.
247

As discussed: I think it would also make sense to merge this with VALID_CONTAINER_ACCESS since those are effectively the same.

258

I think this TODO can be removed now, since it's not an actionable thing anymore.

284–285

I think you can probably just do it in this patch? Or otherwise let's do in a follow-up right after this patch.

libcxx/include/list
1755–1756

Maybe this should be _LIBCPP_ASSERT_VALID_INPUT_RANGE?

libcxx/include/span
245–246

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.

264–265

The same comment applies here and below, those need to be enabled in the hardened mode.

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.

That was not clear to me from the patch. When this is not a "user flag" I feel less strongly about picking the best name. Thanks for the clarification @ldionne.

libcxx/include/__config
235–236

+1

var-const marked 9 inline comments as done.

Partially address feedback and rebase.

_LIBCPP_ASSERT_VALID_ALLOCATOR -> _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR.

Address remaining feedback.

var-const marked 2 inline comments as done.Jul 19 2023, 12:07 PM
var-const added inline comments.
libcxx/include/__config
250

How about _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR?

253

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.

libcxx/include/list
1755–1756

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.

libcxx/include/span
245–246

Done. Decided to go with valid-container-access as it seems to be closer in intention like you mentioned.

264–265

Done throughout span.

ldionne accepted this revision.Jul 19 2023, 12:22 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 19 2023, 12:22 PM
var-const updated this revision to Diff 542210.Jul 19 2023, 2:58 PM

Fix the CI.

Mordante accepted this revision.Jul 20 2023, 9:03 AM

I didn't do another review, but my concerns have been addressed. Since Louis is happy, I'm happy too.

libcxx/include/__config
250

I like that!