This is an archive of the discontinued LLVM Phabricator instance.

DRAFT: hardening "interface"
AbandonedPublic

Authored by var-const on May 28 2023, 8:57 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

var-const created this revision.May 28 2023, 8:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2023, 8:57 PM
var-const updated this revision to Diff 526366.May 28 2023, 8:59 PM

Undo accidental change.

ldionne added inline comments.
libcxx/include/__config_hardening
20–40 ↗(On Diff #526366)

This should move to __config under the ABI selection block. Or while we're at it, I've been meaning to move the ABI checks to somewhere outside of __config for a while but never got to it, so this could be a refactoring we do before adding these new knobs.

Maybe __config_dir/abi.h, __config_dir/assertions.h, __config_dir/availability.h, etc?

61 ↗(On Diff #526366)

Let's create a bug report on github to track this improvement and explain what it is in a few words.

63 ↗(On Diff #526366)

We should have a way to define default values for those in the __config_site so that a vendor can pick a default value for a given platform. This will probably require using 0 and 1 instead of just #ifndef to check for these macros.

63 ↗(On Diff #526366)

We should add documentation for these macros where we document the safe libc++ mode (actually this needs more updating in light of the "new" debug mode).

63 ↗(On Diff #526366)

We should have tests that a user can enable/disable the mode on a per-TU basis -- that's a pretty important property of the design. You can probably take inspiration from libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp.

71 ↗(On Diff #526366)

This is going to conflict with the existing "legacy" debug mode. Can you write a short RFC proposing the removal of the legacy debug mode since it's really causing trouble here and that legacy debug mode has been broken. A few problems about it:

  1. It takes a global lock
  2. It's not constexpr friendly
  3. It hasn't really been maintained (e.g. new features don't necessarily use it)
  4. It was design with the goal of not changing the ABI, but that didn't work because updating the global database is an ABI affecting property (for example if you get a std::string from the dylib which hasn't been built with the debug mode enabled, it won't be in the database so you'll fail if your user code tries to check that the std::string is in the database).
  5. Some vendors (at least Apple) never shipped it
78 ↗(On Diff #526366)

For this one, we probably want to do

// in __config_dir/assertions.h
#ifdef _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY
# define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR
# warning "this is deprecated, use _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR"
#endif

Or maybe we can mark the macro as deprecated with an attribute, it seems to ring a bell but I'm not sure anymore.

84–95 ↗(On Diff #526366)

I think we should just use _LIBCPP_ASSERT here. _LIBCPP_ASSERT isn't tied to the debug mode anymore, it's just a self-standing utility for assertions in libc++ and we should use it universally.

#ifdef _LIBCPP_ENABLE_HARDENING_ASSERTIONS
#  define _LIBCPP_HARDENING_ASSERT(...) _LIBCPP_ASSERT(__VA_ARGS__)
#else
#  define _LIBCPP_HARDENING_ASSERT(...) /* nothing */
#endif
102 ↗(On Diff #526366)

We should force ourselves to put a semicolon after using the macro, otherwise editors will be confused.

libcxx/include/__hardening.h
26 ↗(On Diff #526366)

Should this be part of some kind of __hardening_traits class? Would there be anything else in there other than __refer_to_same_range?

var-const marked an inline comment as done.Jun 1 2023, 9:45 AM
var-const added inline comments.
libcxx/include/__config_hardening
20–40 ↗(On Diff #526366)

Created https://reviews.llvm.org/D151900 to move things under __config_dir (without any other changes for now). We want to move __config itself into the new directory as well, right?

71 ↗(On Diff #526366)
var-const marked 3 inline comments as done.Jun 21 2023, 10:12 AM
var-const added inline comments.
libcxx/include/__config_hardening
61 ↗(On Diff #526366)
var-const updated this revision to Diff 533505.Jun 22 2023, 1:30 AM
var-const marked 6 inline comments as done.

Address more feedback.

var-const updated this revision to Diff 533506.Jun 22 2023, 1:32 AM

hardening.h -> hardening

libcxx/include/__config_hardening
20–40 ↗(On Diff #526366)

Update: this turned out to be a lot harder than initially assumed because the split makes it easier to forget to include a header and mistakenly presume that a macro is undefined. I will postpone D151900 for now and keep everything in the same __config file for the purposes of this patch.

Moved to the ABI section of __config instead.

63 ↗(On Diff #526366)

It looks like the flags I'm passing to the command line get ignored (probably overwritten), I'll need to investigate more to find out why.

libcxx/include/__hardening.h
26 ↗(On Diff #526366)

Done.

My first idea was to rely on overload resolution to find the best match given that _Iter is a dependent name. However, that needs ADL to work which I coincidentally disabled by explicitly qualifying the function name with std::. I think using a traits class is more convenient regardless, thanks for the suggestion!

var-const updated this revision to Diff 533507.Jun 22 2023, 1:36 AM

Quick fix.

var-const added inline comments.Jun 22 2023, 1:44 AM
libcxx/include/__config
88

These are leftovers from testing, but I'm keeping them for now as useful reminders.

92

Note to self: this prevents a compiler warning/error.

286

IIUC, the current state where _LIBCPP_ENABLE_NEW_DEBUG_MODE automatically enables _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS doesn't quite work because then the user cannot use _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS to override the current hardening mode, right? If so, does that mean that the user has to define both e.g. _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS and _LIBCPP_ENABLE_NEW_DEBUG_MODE?

325

I copied this from existing macros. Is there a reason to prefer this form to just /*nothing*/?

libcxx/include/__config_site.in
42

Some notes and questions:

  1. We don't want _ASSERTIONS in the names of the top-level macros, e.g. it should be _LIBCPP_ENABLE_HARDENED_MODE and not _LIBCPP_ASSERTIONS_ENABLE_HARDENED_MODE, right? What about _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS?
  2. I'm not 100% happy with the names _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK and _LIBCPP_ENABLE_HARDENED_MODE. They look obviously related, but the relationship isn't obvious.
  3. For now I went with _LIBCPP_ENABLE_NEW_DEBUG_MODE but this definitely could be improved.
  4. How do CMake definitions from this file interact with what's passed from the command line? I'm getting lots of warnings/errors about macros being undefined or redefined, but I'm probably doing the configuration wrong.
46

Do we want lower-level macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE here?

48

I used the word "feature" here since it's not directly a check, rather it enables a certain behavior. Also, this name is pretty verbose, very open to suggestions on how to shorten it.

libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
117

Looks like this operator wasn't invoked before.

ldionne published this revision for review.Jun 22 2023, 10:23 AM

I really like the shape of this. At a high level, here's what I would recommend:

  1. Extract the ABI macro for bounded iterators out of this patch. This will make this patch smaller and also get that out of the way when you try to remove the legacy debug mode.
  2. Introduce the assertions machinery in a separate patch: _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE, but without any of the check macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS. Yes, that patch is going to look kinda stupid, but that will allow us to properly consider the interaction between those settings and the existing settings we have.
  3. Then, in a separate patch, let's introduce some (or all) of the _LIBCPP_ASSERTIONS_ENABLE_CHECK_foo checks and plumb them into _LIBCPP_ENABLE_HARDENED_MODE/_LIBCPP_ENABLE_NEW_DEBUG_MODE which will already have been introduced.
libcxx/include/__config
196–198

I would suggest not introducing this macro, at least not for now. Instead, I would rename _LIBCPP_ASSERTIONS_ENABLE_HARDENING_BOUNDED_ITERATORS to _LIBCPP_ABI_BOUNDED_ITERATORS, a normal ABI macro like we have others and I would simply not define it in any configuration by default. In fact, this could be done in a separate patch to simplify this one quite a bit.

Actually if you do that in a separate patch, _LIBCPP_ABI_BOUNDED_ITERATORS could take over _LIBCPP_DEBUG_ITERATOR_BOUNDS_CHECKING.

242

Since this affects both hardening and "debug" checks, I don't think _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS is the right name. Is this different from _LIBCPP_ENABLE_ASSERTIONS?

Answer: The difference between this macro and _LIBCPP_ENABLE_ASSERTIONS is that the latter also disables some currently-uncategorized assertions.

IMO it's not worth introducing another public knob for this, I think people don't need that much granularity. I can't think of anyone wanting to disable hardening and debug assertions while somehow keeping all the "uncategorized" assertions.

274–276

Suggestion: instead, you could do this outside of the #if block:

#if _LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_NEW_DEBUG_MODE
# error "Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE can be defined."
#endif
316

Let's fully qualify everything in macros.

325

I think it's to make this valid to use in an expression. I have a test that _LIBCPP_ASSERT can be used in an expression, and we should have similar tests for new assertion macros being introduced (possibly add to the same test).

libcxx/include/__config_site.in
42

We don't want _ASSERTIONS in the names of the top-level macros, e.g. it should be _LIBCPP_ENABLE_HARDENED_MODE and not _LIBCPP_ASSERTIONS_ENABLE_HARDENED_MODE, right?

I would agree with that.

I'm not 100% happy with the names _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK and _LIBCPP_ENABLE_HARDENED_MODE. They look obviously related, but the relationship isn't obvious.

As I suggested, I think we can remove _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK entirely, at least for now. This makes the issue moot.

46

I don't think it makes sense for those to be settable at configure time in the __config_site.in. I think I would only define the default value we want for the debug and hardened mode in here. Basically, vendors could decide whether they want a specific build of libc++ to enable the debug/hardened mode by default (users could still override it) by setting it in CMake. For example you might want to do:

#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
#cmakedefine01 _LIBCPP_ENABLE_DEBUG_MODE_DEFAULT

Those would be set from CMake and would provide the default value to use for _LIBCPP_ENABLE_HARDENED_MODE resp _LIBCPP_ENABLE_DEBUG_MODE inside __config.

libcxx/include/__debug_utils/randomize_range.h
14

To reduce the size of this patch, I would suggest that we "onboard" features into the hardened/debug mode in separate patches. This patch can introduce the basic functionality, but then we can have a separate patch that simply moves _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY from the old debug mode into the "new debug mode".

I think doing those piecemeal is going to make this patch much easier to review, but also give us space to consider important things like transitions and temporary backwards compatibility for existing macros.

libcxx/include/span
241

I find this potentially confusing. The name _LIBCPP_ASSERT_CHECK_INVALID_RANGE makes it look as if we're checking that the range is invalid, when we're trying to do the reverse. Should this be called _LIBCPP_ASSERT_CHECK_VALID_RANGE instead (and similarly for other types of checks)?

libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
117

Let's land this as a NFC before this patch. Actually, is there a reason for defining it at all if it's not used?

Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 10:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 534755.Jun 26 2023, 2:49 PM
var-const marked 3 inline comments as done.

Addressing feedback wip.

var-const updated this revision to Diff 534833.Jun 26 2023, 9:50 PM
var-const marked 3 inline comments as done.

Address more feedback

libcxx/include/__config
242

https://reviews.llvm.org/D153816 (s/_LIBCPP_ASSERT/_LIBCPP_ASSERT_UNCATEGORIZED).

325

Do we need to add those tests considering that we deliberately define the new assertions to expand to _LIBCPP_ASSERT which is already thoroughly tested in this regard?

libcxx/include/span
241

I think it depends on how the reader sees the purpose of the check. I'm looking at it from the perspective of "which condition will cause the assertion to trigger?" (and more broadly, "what kind of bad behavior are we providing guarantees against?"). Or to put it simpler, I'm reading this kinda like "abort if the range is invalid", which I find slightly more natural than "make sure the range is valid and proceed".

I don't feel too strongly about this and can be convinced to change. One argument in favor of "check valid range" is that it follows the semantics of "assert" (which fires if the condition is false, not true).

EDIT: after some thinking, I went ahead with the renaming as suggested in this comment.

libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
117

Done. It's used by some of the hardening checks.

var-const updated this revision to Diff 535035.Jun 27 2023, 9:58 AM

Remove commented-out code.

var-const marked an inline comment as done.

BAD_CONTAINER_ACCESS -> VALID_CONTAINER_ACCESS

Thanks for working on this!

libcxx/include/__config
88

Would it make sense to try to land the __config split patch first?

206

Is it intended to add all containers in the future?

223

I really dislike names with _NEW_ in it, it tends to get stale at some point. Just like terms as "modern C++" which some people feel is C++11...

I would suggest _LIBCPP_ENABLE_HARDENED_DEBUG_MODE instead. Or would it make sense to do
//#define _LIBCPP_ENABLE_HARDENED_MODE 1 as is
//#define _LIBCPP_ENABLE_HARDENED_MODE 2 the debug mode. This automatically makes them mutually exclusive.

238
244

Can you change them to TODO HARDENING or similar to make grepping them easier.

253

Thanks a lot for the documentation! It really helps to understand what the hardening mode will do.

var-const added inline comments.Jun 27 2023, 10:49 AM
libcxx/include/__config
196–198

Patch for introducing _LIBCPP_ABI_BOUNDED_ITERATORS: https://reviews.llvm.org/D153895

  1. Extract the ABI macro for bounded iterators out of this patch. This will make this patch smaller and also get that out of the way when you try to remove the legacy debug mode.

https://reviews.llvm.org/D153895

  1. Introduce the assertions machinery in a separate patch: _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE, but without any of the check macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS. Yes, that patch is going to look kinda stupid, but that will allow us to properly consider the interaction between those settings and the existing settings we have.

https://reviews.llvm.org/D153902

  1. Then, in a separate patch, let's introduce some (or all) of the _LIBCPP_ASSERTIONS_ENABLE_CHECK_foo checks and plumb them into _LIBCPP_ENABLE_HARDENED_MODE/_LIBCPP_ENABLE_NEW_DEBUG_MODE which will already have been introduced.

I'll reuse this patch once the other two patches have landed.

libcxx/include/__config
223

My hope is that I can land https://reviews.llvm.org/D153672 first which would allow me to reuse the "obvious" name _LIBCPP_ENABLE_DEBUG_MODE. Otherwise, I like _LIBCPP_ENABLE_HARDENED_DEBUG_MODE -- I wasn't very happy with the "new" name either, so thanks a lot for the suggestion!

Re. using 1 and 2 as levels -- I agree it automatically makes them mutually exclusive which is a good property to have. On the other hand, having named macros is perhaps a little more readable than using numbers to represent each mode -- it's not obvious without reading the documentation what exactly _LIBCPP_ENABLE_HARDENED_MODE 2 means (and whether 2 is the max value or not).

Following @ldionne's feedback, I created a new patch that only adds the configuration macros, without any assertions, and used the name _LIBCPP_ENABLE_HARDENED_DEBUG_MODE there: https://reviews.llvm.org/D153902

var-const abandoned this revision.Aug 31 2023, 7:08 PM

This patch is no longer relevant -- most of its contents have been merged in other patches.