You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/10/09 04:43:17 UTC
Review Request 71599: Added validation against scalar overflow.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71599/
-----------------------------------------------------------
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
Re: Review Request 71599: Added validation against scalar overflow.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71599/#review218151
-----------------------------------------------------------
Patch looks great!
Reviews applied: [71595, 71596, 71597, 71598, 71599]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 71599: Added validation against scalar overflow.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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
>
>