----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71599/#review218153 -----------------------------------------------------------
src/common/validation.cpp Lines 686 (patched) <https://reviews.apache.org/r/71599/#comment305724> I'm afraid this is not portable. The standard says that the value returned by `std::llrand()` when its result would be outside of `long long` range is unspecified. For example, this would break for sure on a platform where `llrand` implementation does something like a wrap-on-overflow or returns 0 on overflow. Digged into this a bit - looks like the only sane option is to compare the value against the maximum valid scalar. On an IEC559/IEEE754-compliant platform (basically every platform of our interest, I guess) this is slightly larger than ` static_assert(numeric_limits<double>::is_iec559); const double max_valid_scalar = static_cast<double>( static_cast<unsigned long long>(numeric_limits<long long>::max()) & ~(1ul << (numeric_limits<long long>::digits - numeric_limits<double>::digits) - 1ul) ) / 1000.0; ` ("figuring out the exact value" seems to make no sense, since there are only 53 significant bits in a 64 bit `double`) Note: on the compliant platforms `llround()` should set floating point exceptions if the result would be out of range. In theory, this could be tested for via `std::fetestexcept()`. However, in practice compilers do not treat the FP exceptions as an effect - see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38960 bug - and thus they can and do reorder `llround` and `fetestexcept` (observed this on gcc 7.4.0 with -O2). - Andrei Sekretenko On Oct. 9, 2019, 4:43 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71599/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2019, 4:43 a.m.) > > > Review request for mesos, Andrei Sekretenko and Meng Zhu. > > > Repository: mesos > > > Description > ------- > > Note that this input scalar validation is currently used during quota > validation as well as during parsing. This means resources passed as > strings will now include this overflow validation. > > This validation is insufficient, it's still possible to use a very > large value close to the overflow point, and have some addition occur > later leading to overflow and a crash. But, it's better than nothing. > > > Diffs > ----- > > src/common/validation.cpp 14a8c7b9d70c303b527e5e43012999471ced2d78 > src/common/values.cpp 75203824a1016eae7088bee19e61ee8cf8d3a660 > src/v1/values.cpp 6193dbd8bad7f060e6bb2c18e3984c4a4b061ab6 > > > Diff: https://reviews.apache.org/r/71599/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
