guard non-finite cast in log_uniform_int_distribution param_type - #2144
guard non-finite cast in log_uniform_int_distribution param_type#2144nabhan06 wants to merge 1 commit into
Conversation
derekmauro
left a comment
derekmauro
left a comment
There was a problem hiding this comment.
/cc @laramiel
There is a fundamental difference between C++ language-level UB (compiler optimizations exploiting float overflow, which we want to avoid) and API contract violations.
Contract violations should fail fast and loudly. It should be very uncomfortable to have such a violation, otherwise users will eventually come to depend on them (Hyrum's Law).
We should avoid the language UB so UBSan and optimizers remain happy, but we should not promote an out-of-contract state into a tested feature.
| // value which can be used to construct bounds. | ||
| log_range_ = (std::min)(random_internal::BitWidth(range()), | ||
| std::numeric_limits<unsigned_type>::digits); | ||
| } else { |
There was a problem hiding this comment.
How about instead we simply change this to base_ > 2? This avoids all language level UB.
| @@ -80,7 +80,18 @@ | |||
| // which can eliminate some values depending on where the bounds fall. | |||
| const double inv_log_base = 1.0 / std::log(static_cast<double>(base_)); | |||
There was a problem hiding this comment.
Note that this fix fails to handle base_ == 1, which will produce a divide by 0 here.
| range_(static_cast<unsigned_type>(max_) - | ||
| static_cast<unsigned_type>(min_)), | ||
| log_range_(0) { | ||
| assert(max_ >= min_); |
There was a problem hiding this comment.
We should use ABSL_HARDENING_ASSERT to check preconditions.
| EXPECT_GE(sample, dist.min()); | ||
| EXPECT_LE(sample, dist.max()); | ||
|
|
||
| // The same bad base arriving through deserialization must also stay defined. |
There was a problem hiding this comment.
The proper fix for this is for operator>> to validate input and set std::ios_base::failbit. The wrong fix is for param_type to quietly sanitize bad input and leave the distribution in a silently failing state.
2 participants
log_uniform_int_distribution's param_type constructor computes log_range_ as static_cast(ceil((1/log(base)) * log(range))). A base of 0 or 1, or a negative base for a signed IntType, breaks the base > 1 precondition and makes 1/log(base) non-finite, so the value fed to the int cast is inf or NaN and the cast is undefined behavior; UBSan flags it as "inf is outside the range of representable values of type 'int'" at log_uniform_int_distribution.h:83. It is reachable by direct construction and also by operator>>, which reads base straight from the stream, and it contradicts the opt-mode contract that invalid params yield a defined value rather than UB. Guard the cast so an out-of-contract base produces a defined result while valid bases (> 1) stay unchanged.