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.
There was a problem hiding this comment.
How about instead we simply change this to base_ > 2? This avoids all language level UB.
There was a problem hiding this comment.
Done, the else is now else if (base_ > 2) and the cast guard is gone.
| // | ||
| // Thus a result which should equal K may equal K +/- epsilon, | ||
| // 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.
There was a problem hiding this comment.
Right, the guard only covered the cast. With the branch gated on base_ > 2 the log() math is not reached for base 1 (or 0 / negative) at all. Verified with -fsanitize=float-divide-by-zero on top of the usual UBSan checks.
There was a problem hiding this comment.
We should use ABSL_HARDENING_ASSERT to check preconditions.
There was a problem hiding this comment.
Done, both preconditions use ABSL_HARDENING_ASSERT now.
| 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.
There was a problem hiding this comment.
Makes sense. operator>> now checks max >= min and base > 1 on the parsed values and sets failbit without touching the distribution. The opt-mode test is gone; the replacement only checks that operator>> rejects that input.
…itions Address review feedback: check the param_type preconditions with ABSL_HARDENING_ASSERT, only take the log() branch for base_ > 2 so an out-of-contract base never reaches floating-point undefined behavior (including the divide by log(1) == 0), and have operator>> validate max >= min and base > 1, setting failbit instead of constructing an invalid param_type. Replace the opt-mode out-of-contract test with one that checks operator>> rejects such input.
|
Agreed on the contract vs. language UB distinction. Pushed a revision along those lines: ABSL_HARDENING_ASSERT for the preconditions, the log() branch gated on base_ > 2 with the cast guard dropped, and operator>> validating and setting failbit instead of handing bad input to param_type. The test no longer exercises out-of-contract construction, it just covers the operator>> rejection. Passes in fastbuild and -c opt with UBSan (including float-divide-by-zero). |
nabhan06
changed the title
3 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, violates the base > 1 precondition and makes that math non-finite: 1/log(1) divides by zero, and the final cast of inf/NaN to int is undefined behavior (UBSan: "inf is outside the range of representable values of type 'int'"). It was also reachable through operator>>, which handed whatever it parsed straight to param_type.
Changes: