[libc++][hardening] Categorize assertions that produce incorrect results #77183
Conversation
Introduce a new `argument-within-domain` category that covers cases where the given arguments make it impossible to produce a correct result (or create a valid object in case of constructors). While the incorrect result doesn't create an immediate problem within the library (like e.g. a null pointer dereference would), it always indicates a logic error in user code and is highly likely to lead to a bug in the program once the value is used.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Konstantin Varlamov (var-const) ChangesIntroduce a new Full diff: https://github.com/llvm/llvm-project/pull/77183.diff 13 Files Affected:
diff --git a/libcxx/include/__algorithm/clamp.h b/libcxx/include/__algorithm/clamp.h
index 1631b2673c3faf..003bf70dd4f01d 100644
--- a/libcxx/include/__algorithm/clamp.h
+++ b/libcxx/include/__algorithm/clamp.h
@@ -26,7 +26,7 @@ clamp(_LIBCPP_LIFETIMEBOUND const _Tp& __v,
_LIBCPP_LIFETIMEBOUND const _Tp& __lo,
_LIBCPP_LIFETIMEBOUND const _Tp& __hi,
_Compare __comp) {
- _LIBCPP_ASSERT_UNCATEGORIZED(!__comp(__hi, __lo), "Bad bounds passed to std::clamp");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(!__comp(__hi, __lo), "Bad bounds passed to std::clamp");
return __comp(__v, __lo) ? __lo : __comp(__hi, __v) ? __hi : __v;
}
diff --git a/libcxx/include/__algorithm/ranges_clamp.h b/libcxx/include/__algorithm/ranges_clamp.h
index e6c86207254a19..a1185e7278f0ed 100644
--- a/libcxx/include/__algorithm/ranges_clamp.h
+++ b/libcxx/include/__algorithm/ranges_clamp.h
@@ -34,8 +34,9 @@ struct __fn {
indirect_strict_weak_order<projected<const _Type*, _Proj>> _Comp = ranges::less>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr const _Type& operator()(
const _Type& __value, const _Type& __low, const _Type& __high, _Comp __comp = {}, _Proj __proj = {}) const {
- _LIBCPP_ASSERT_UNCATEGORIZED(!bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
- "Bad bounds passed to std::ranges::clamp");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
+ !bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
+ "Bad bounds passed to std::ranges::clamp");
auto&& __projected = std::invoke(__proj, __value);
if (std::invoke(__comp, std::forward<decltype(__projected)>(__projected), std::invoke(__proj, __low)))
diff --git a/libcxx/include/__bit/bit_ceil.h b/libcxx/include/__bit/bit_ceil.h
index 17fe06aa41ccd8..77fa739503bc58 100644
--- a/libcxx/include/__bit/bit_ceil.h
+++ b/libcxx/include/__bit/bit_ceil.h
@@ -28,7 +28,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Tp __bit_ceil(_Tp __t) no
if (__t < 2)
return 1;
const unsigned __n = numeric_limits<_Tp>::digits - std::__countl_zero((_Tp)(__t - 1u));
- _LIBCPP_ASSERT_UNCATEGORIZED(__n != numeric_limits<_Tp>::digits, "Bad input to bit_ceil");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__n != numeric_limits<_Tp>::digits, "Bad input to bit_ceil");
if constexpr (sizeof(_Tp) >= sizeof(unsigned))
return _Tp{1} << __n;
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 082c73e672c749..9ba4fce834132f 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -283,6 +283,14 @@
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
// the containers have compatible allocators.
//
+// - `_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN` -- checks that the given argument is within the domain of valid arguments
+// for the function. Violating this typically produces an incorrect result (e.g. the clamp algorithm returns the
+// original value without clamping it due to incorrect functors) or puts an object into an invalid state (e.g.
+// a string view where only a subset of elements is possible to access). This doesn't cause an immediate issue within
+// the library but is always a logic bug and is likely to cause problems within user code.
+// This is somewhat of a catch-all (or fallback) category -- it covers errors triggered by user input that don't have
+// a more specific category defined (which is always preferable when available).
+//
// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
// be benign in our implementation.
//
@@ -327,6 +335,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
// Overlapping ranges will make algorithms produce incorrect results but don't directly lead to a security
// vulnerability.
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
@@ -342,8 +351,9 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
-# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
// Disabled checks.
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
@@ -357,6 +367,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
@@ -371,6 +382,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 4ca49fe42606c7..13420012006e2d 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -861,7 +861,7 @@ public:
template <class _Key>
_LIBCPP_HIDE_FROM_ABI size_type bucket(const _Key& __k) const {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
bucket_count() > 0, "unordered container::bucket(key) called when bucket_count() == 0");
return std::__constrain_hash(hash_function()(__k), bucket_count());
}
diff --git a/libcxx/include/__memory/assume_aligned.h b/libcxx/include/__memory/assume_aligned.h
index c66fb49ebb3c01..5036df7f0f5d5c 100644
--- a/libcxx/include/__memory/assume_aligned.h
+++ b/libcxx/include/__memory/assume_aligned.h
@@ -29,7 +29,8 @@ _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp* __ass
if (__libcpp_is_constant_evaluated()) {
return __ptr;
} else {
- _LIBCPP_ASSERT_UNCATEGORIZED(reinterpret_cast<uintptr_t>(__ptr) % _Np == 0, "Alignment assumption is violated");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
+ reinterpret_cast<uintptr_t>(__ptr) % _Np == 0, "Alignment assumption is violated");
return static_cast<_Tp*>(__builtin_assume_aligned(__ptr, _Np));
}
}
diff --git a/libcxx/include/__numeric/gcd_lcm.h b/libcxx/include/__numeric/gcd_lcm.h
index 3e9c244f25c285..48df2338051e29 100644
--- a/libcxx/include/__numeric/gcd_lcm.h
+++ b/libcxx/include/__numeric/gcd_lcm.h
@@ -77,7 +77,7 @@ _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI common_type_t<_Tp, _Up> lcm(_Tp __m, _Up
using _Rp = common_type_t<_Tp, _Up>;
_Rp __val1 = __ct_abs<_Rp, _Tp>()(__m) / std::gcd(__m, __n);
_Rp __val2 = __ct_abs<_Rp, _Up>()(__n);
- _LIBCPP_ASSERT_UNCATEGORIZED((numeric_limits<_Rp>::max() / __val1 > __val2), "Overflow in lcm");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN((numeric_limits<_Rp>::max() / __val1 > __val2), "Overflow in lcm");
return __val1 * __val2;
}
diff --git a/libcxx/include/barrier b/libcxx/include/barrier
index fcfc96cb0484cf..c3833630461cff 100644
--- a/libcxx/include/barrier
+++ b/libcxx/include/barrier
@@ -128,7 +128,7 @@ public:
__completion_(std::move(__completion)),
__phase_(0) {}
[[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI arrival_token arrive(ptrdiff_t __update) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__update <= __expected_, "update is greater than the expected count for the current barrier phase");
auto const __old_phase = __phase_.load(memory_order_relaxed);
@@ -186,7 +186,7 @@ public:
auto const __result = __arrived.fetch_sub(update, memory_order_acq_rel) - update;
auto const new_expected = __expected.load(memory_order_relaxed);
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
update <= new_expected, "update is greater than the expected count for the current barrier phase");
if (0 == __result) {
@@ -231,7 +231,7 @@ public:
auto const __inc = __arrived_unit * update;
auto const __old = __phase_arrived_expected.fetch_add(__inc, memory_order_acq_rel);
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
update <= __old, "update is greater than the expected count for the current barrier phase");
if ((__old ^ (__old + __inc)) & __phase_bit) {
@@ -267,10 +267,10 @@ public:
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI explicit barrier(
ptrdiff_t __count, _CompletionF __completion = _CompletionF())
: __b_(__count, std::move(__completion)) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__count >= 0,
"barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with a negative value");
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__count <= max(),
"barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with "
"a value greater than max()");
@@ -280,7 +280,7 @@ public:
barrier& operator=(barrier const&) = delete;
[[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI arrival_token arrive(ptrdiff_t __update = 1) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__update > 0, "barrier:arrive must be called with a value greater than 0");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update > 0, "barrier:arrive must be called with a value greater than 0");
return __b_.arrive(__update);
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void wait(arrival_token&& __phase) const {
diff --git a/libcxx/include/latch b/libcxx/include/latch
index ef52c0562a7c51..e7cc7d0e4b347d 100644
--- a/libcxx/include/latch
+++ b/libcxx/include/latch
@@ -72,11 +72,11 @@ public:
static _LIBCPP_HIDE_FROM_ABI constexpr ptrdiff_t max() noexcept { return numeric_limits<ptrdiff_t>::max(); }
inline _LIBCPP_HIDE_FROM_ABI constexpr explicit latch(ptrdiff_t __expected) : __a_(__expected) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__expected >= 0,
"latch::latch(ptrdiff_t): latch cannot be "
"initialized with a negative value");
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__expected <= max(),
"latch::latch(ptrdiff_t): latch cannot be "
"initialized with a value greater than max()");
@@ -87,9 +87,9 @@ public:
latch& operator=(const latch&) = delete;
inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void count_down(ptrdiff_t __update = 1) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__update >= 0, "latch::count_down called with a negative value");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update >= 0, "latch::count_down called with a negative value");
auto const __old = __a_.fetch_sub(__update, memory_order_release);
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__update <= __old,
"latch::count_down called with a value greater "
"than the internal counter");
@@ -101,7 +101,7 @@ public:
__cxx_atomic_wait(&__a_.__a_, [this]() -> bool { return try_wait(); });
}
inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void arrive_and_wait(ptrdiff_t __update = 1) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__update >= 0, "latch::arrive_and_wait called with a negative value");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update >= 0, "latch::arrive_and_wait called with a negative value");
// other preconditions on __update are checked in count_down()
count_down(__update);
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index de45b8b5db1014..87e3b31fd2b164 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -91,7 +91,7 @@ public:
_LIBCPP_HIDE_FROM_ABI constexpr explicit __atomic_semaphore_base(ptrdiff_t __count) : __a_(__count) {}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void release(ptrdiff_t __update = 1) {
auto __old = __a_.fetch_add(__update, memory_order_release);
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
if (__old > 0) {
@@ -137,11 +137,11 @@ public:
static constexpr ptrdiff_t max() noexcept { return __least_max_value; }
_LIBCPP_HIDE_FROM_ABI constexpr explicit counting_semaphore(ptrdiff_t __count) : __semaphore_(__count) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__count >= 0,
"counting_semaphore::counting_semaphore(ptrdiff_t): counting_semaphore cannot be "
"initialized with a negative value");
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__count <= max(),
"counting_semaphore::counting_semaphore(ptrdiff_t): counting_semaphore cannot be "
"initialized with a value greater than max()");
@@ -152,7 +152,7 @@ public:
counting_semaphore& operator=(const counting_semaphore&) = delete;
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void release(ptrdiff_t __update = 1) {
- _LIBCPP_ASSERT_UNCATEGORIZED(__update >= 0, "counting_semaphore:release called with a negative value");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update >= 0, "counting_semaphore:release called with a negative value");
__semaphore_.release(__update);
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() { __semaphore_.acquire(); }
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 909224fe7e3d03..e414507a7933b6 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -307,8 +307,11 @@ public:
: __data_(__s),
__size_(__len) {
#if _LIBCPP_STD_VER >= 14
- _LIBCPP_ASSERT_UNCATEGORIZED(__len <= static_cast<size_type>(numeric_limits<difference_type>::max()),
- "string_view::string_view(_CharT *, size_t): length does not fit in difference_type");
+ // This will result in creating an invalid `string_view` object -- some calculations involving `size` would
+ // overflow, making it effectively truncated.
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
+ __len <= static_cast<size_type>(numeric_limits<difference_type>::max()),
+ "string_view::string_view(_CharT *, size_t): length does not fit in difference_type");
_LIBCPP_ASSERT_NON_NULL(
__len == 0 || __s != nullptr, "string_view::string_view(_CharT *, size_t): received nullptr");
#endif
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 6bee340e0d15c8..8a7d6cc94c7568 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -608,10 +608,9 @@ void __permissions(const path& p, perms prms, perm_options opts, error_code* ec)
const bool resolve_symlinks = !has_opt(perm_options::nofollow);
const bool add_perms = has_opt(perm_options::add);
const bool remove_perms = has_opt(perm_options::remove);
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
(add_perms + remove_perms + has_opt(perm_options::replace)) == 1,
- "One and only one of the perm_options constants replace, add, or remove "
- "is present in opts");
+ "One and only one of the perm_options constants 'replace', 'add', or 'remove' must be present in opts");
bool set_sym_perms = false;
prms &= perms::mask;
diff --git a/libcxx/src/include/to_chars_floating_point.h b/libcxx/src/include/to_chars_floating_point.h
index e4715d10d97dac..01c26181697b7b 100644
--- a/libcxx/src/include/to_chars_floating_point.h
+++ b/libcxx/src/include/to_chars_floating_point.h
@@ -994,7 +994,7 @@ to_chars_result _Floating_to_chars(
if constexpr (_Overload == _Floating_to_chars_overload::_Plain) {
_LIBCPP_ASSERT_INTERNAL(_Fmt == chars_format{}, ""); // plain overload must pass chars_format{} internally
} else {
- _LIBCPP_ASSERT_UNCATEGORIZED(_Fmt == chars_format::general || _Fmt == chars_format::scientific
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(_Fmt == chars_format::general || _Fmt == chars_format::scientific
|| _Fmt == chars_format::fixed || _Fmt == chars_format::hex,
"invalid format in to_chars()");
}
|
| @@ -307,8 +307,11 @@ public: | |||
| : __data_(__s), | |||
| __size_(__len) { | |||
| #if _LIBCPP_STD_VER >= 14 | |||
| _LIBCPP_ASSERT_UNCATEGORIZED(__len <= static_cast<size_type>(numeric_limits<difference_type>::max()), | |||
| "string_view::string_view(_CharT *, size_t): length does not fit in difference_type"); | |||
| // This will result in creating an invalid `string_view` object -- some calculations involving `size` would | |||
There was a problem hiding this comment.
Note: while a few of these involve integer overflow, I don't think it's a good criterion for classification. To me, it seems very implementation-centric -- the fact that there is an overflow involved doesn't say much about the actual issue that will happen, which can range from something very minor or even benign to compromizing the memory safety of the program.
There was a problem hiding this comment.
I think this one has more serious consequences than the categorization and comment suggest. The size parameter determines the bounds of the string. Every byte from __s[0] up to __s[__len - 1] is fair game for the program to access. E.g. the bounds checks in operator[] assume the length is correct.
It is not possible for a length over PTRDIFF_MAX to be the correct bounds for __s. No allocation can exceed that amount. Moreover, it's not hard for a program to accidentally construct such a string_view by accidentally underflowing a computation and passing a negative number. That negative number will, in turn, be read as SIZE_MAX.
See #61100 for context.
| @@ -283,6 +283,14 @@ | |||
| // - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure | |||
| // the containers have compatible allocators. | |||
| // | |||
| // - `_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN` -- checks that the given argument is within the domain of valid arguments | |||
There was a problem hiding this comment.
I spent a lot of time thinking how to name this category and I can't say I'm completely satisfied with the result. The criteria I'm using is roughly "The given arguments make it impossible for us to produce the correct result; returning a nonsensical value doesn't cause any immediate runtime issues within our library, but is likely to lead to a bug, potentially a serious one, in the user code once they access the value".
Intuitively, it does seem like a well-defined concept to me. I think the difficulty I experienced with the naming is due to the fact that our naming scheme mostly describes the expectation for the condition, whereas my intuitive definition focuses on the outcomes of violating the condition. It's not an issue when there is a direct one-to-one correspondence between the two, which I think is the case for most other category names (e.g. valid-element-access arguably describes both the cause and the effect because there is a direct correspondence between them). This, however, is more of a many-to-one relationship. An obvious name would be valid-argument, but since we're mostly checking user-provided arguments, this really applies to every category we have -- for example, valid-element-access is ultimately also a check for a valid argument. I still think that a naming scheme based on the outcomes being prevented would be easier to work with, but it goes against the fact that asserting asserts the successful case, not the failing case.
While argument-within-domain is not that different from invalid-argument, it seems a little more specific to me and also seems to apply to the existing usages pretty well (it is, of course, inspired by std::domain_error). I thought about something like valid-argument-fallback or nonspecific-valid-argument but it doesn't really help better express the part I consider the more important one, giving the user back an incorrect result (or object, which can be seen as a result of running a constructor).
Another aspect to consider here is how easy it would be for a library developer, especially someone who is new to libc++, to use the new category in the intended way.
There was a problem hiding this comment.
Can we have a guide about the different categories? Even a link to the definitions would give a clearer context. Would that make sense? -> https://libcxx.llvm.org/Hardening.html
There was a problem hiding this comment.
Yes, absolutely. I want to finish the actual classification before writing it down -- I think it would be easier to describe it and generalize once the design is finalized. There are only a few dozen unclassified assertions left (not counting in-flight patches like this one), so I hope things settle down pretty soon on that front.
There was a problem hiding this comment.
Thanks for the writeup, this is really useful. Naming this is indeed really tricky. Some suggestions:
_LIBCPP_ASSERT_CANT_MEET_POSTCONDITIONS_LIBCPP_ASSERT_CONSEQUENCES_UNKNOWN_LIBCPP_ASSERT_CONSEQUENCES_OUTSIDE_LIBRARY
None of those is really pretty, but that's some food for thought. I think the unifying theme here is that the assertion is about times when you can't meet your postconditions, not about a "fallback category". The documentation should probably be updated not to give the impression that this is a fallback category.
| @@ -283,6 +283,14 @@ | |||
| // - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure | |||
| // the containers have compatible allocators. | |||
| // | |||
| // - `_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN` -- checks that the given argument is within the domain of valid arguments | |||
There was a problem hiding this comment.
Thanks for the writeup, this is really useful. Naming this is indeed really tricky. Some suggestions:
_LIBCPP_ASSERT_CANT_MEET_POSTCONDITIONS_LIBCPP_ASSERT_CONSEQUENCES_UNKNOWN_LIBCPP_ASSERT_CONSEQUENCES_OUTSIDE_LIBRARY
None of those is really pretty, but that's some food for thought. I think the unifying theme here is that the assertion is about times when you can't meet your postconditions, not about a "fallback category". The documentation should probably be updated not to give the impression that this is a fallback category.
Introduce a new
argument-within-domaincategory that covers caseswhere the given arguments make it impossible to produce a correct result
(or create a valid object in case of constructors). While the incorrect
result doesn't create an immediate problem within the library (like e.g.
a null pointer dereference would), it always indicates a logic error in
user code and is highly likely to lead to a bug in the program once the
value is used.