Commit 47868e49 authored by Marcus Holland-Moritz's avatar Marcus Holland-Moritz Committed by Facebook Github Bot 3

Fix undefined behaviour in float<->int conversion

Summary:
This change fixes a case of undefined behaviour when converting between
floating point and integral values using folly::to<>. Undefined behaviour
is triggered when a floating point value is cast into an integral value
that cannot represent the source value. This happens in both cases,
float-to-int and int-to-float conversion, with folly::to<> due to the
check for loss of precision.

The new test cases expose the undefined behaviour.

The fix is a series of extra checks, the majority of which will only
kick in if the value to be converted is close to the boundary of the
range of the target type. These checks ensure that the conversion will
only be performed if the source value is within the range representable
by the target type.

The extra checks /will/ make the code slower. However, a later change
in this series, which refactors the implementation of folly::to<>, will
restore some of the performance.

Reviewed By: yfeldblum

Differential Revision: D3433757

fbshipit-source-id: 43495d18f831206ef48f74332663263d789a4f8a
parent c1d4dcfe
...@@ -1160,6 +1160,52 @@ parseTo(StringPiece src, Tgt& out) { ...@@ -1160,6 +1160,52 @@ parseTo(StringPiece src, Tgt& out) {
* Integral to floating point and back * Integral to floating point and back
******************************************************************************/ ******************************************************************************/
namespace detail {
/**
* Check if a floating point value can safely be converted to an
* integer value without triggering undefined behaviour.
*/
template <typename Tgt, typename Src>
typename std::enable_if<
std::is_floating_point<Src>::value && std::is_integral<Tgt>::value,
bool>::type
inline checkConversion(const Src& value) {
constexpr Src tgtMaxAsSrc = static_cast<Src>(std::numeric_limits<Tgt>::max());
constexpr Src tgtMinAsSrc = static_cast<Src>(std::numeric_limits<Tgt>::min());
if (value >= tgtMaxAsSrc) {
if (value > tgtMaxAsSrc) {
return false;
}
const Src mmax = std::nextafter(tgtMaxAsSrc, Src());
if (static_cast<Tgt>(value - mmax) >
std::numeric_limits<Tgt>::max() - static_cast<Tgt>(mmax)) {
return false;
}
} else if (std::is_signed<Tgt>::value && value <= tgtMinAsSrc) {
if (value < tgtMinAsSrc) {
return false;
}
const Src mmin = std::nextafter(tgtMinAsSrc, Src());
if (static_cast<Tgt>(value - mmin) <
std::numeric_limits<Tgt>::min() - static_cast<Tgt>(mmin)) {
return false;
}
}
return true;
}
// Integers can always safely be converted to floating point values
template <typename Tgt, typename Src>
typename std::enable_if<
std::is_integral<Src>::value && std::is_floating_point<Tgt>::value,
bool>::type
checkConversion(const Src&) {
return true;
}
}
/** /**
* Checked conversion from integral to floating point and back. The * Checked conversion from integral to floating point and back. The
* result must be convertible back to the source type without loss of * result must be convertible back to the source type without loss of
...@@ -1174,9 +1220,15 @@ typename std::enable_if< ...@@ -1174,9 +1220,15 @@ typename std::enable_if<
(std::is_floating_point<Src>::value && std::is_integral<Tgt>::value), (std::is_floating_point<Src>::value && std::is_integral<Tgt>::value),
Tgt>::type Tgt>::type
to(const Src & value) { to(const Src & value) {
if (detail::checkConversion<Tgt>(value)) {
Tgt result = Tgt(value); Tgt result = Tgt(value);
if (detail::checkConversion<Src>(result)) {
auto witness = static_cast<Src>(result); auto witness = static_cast<Src>(result);
if (value != witness) { if (value == witness) {
return result;
}
}
}
throw std::range_error( throw std::range_error(
to<std::string>("to<>: loss of precision when converting ", value, to<std::string>("to<>: loss of precision when converting ", value,
#ifdef FOLLY_HAS_RTTI #ifdef FOLLY_HAS_RTTI
...@@ -1185,8 +1237,6 @@ to(const Src & value) { ...@@ -1185,8 +1237,6 @@ to(const Src & value) {
" to other type" " to other type"
#endif #endif
).c_str()); ).c_str());
}
return result;
} }
/******************************************************************************* /*******************************************************************************
......
...@@ -792,6 +792,37 @@ TEST(Conv, StringToBool) { ...@@ -792,6 +792,37 @@ TEST(Conv, StringToBool) {
EXPECT_EQ(buf5, sp5.begin()); EXPECT_EQ(buf5, sp5.begin());
} }
TEST(Conv, FloatToInt) {
EXPECT_EQ(to<int>(42.0f), 42);
EXPECT_EQ(to<int8_t>(-128.0f), int8_t(-128));
EXPECT_THROW(to<int8_t>(-129.0), std::range_error);
EXPECT_THROW(to<int8_t>(127.001), std::range_error);
EXPECT_THROW(to<uint8_t>(-0.0001), std::range_error);
EXPECT_THROW(
to<uint64_t>(static_cast<float>(std::numeric_limits<uint64_t>::max())),
std::range_error);
}
TEST(Conv, IntToFloat) {
EXPECT_EQ(to<float>(42ULL), 42.0);
EXPECT_EQ(to<float>(int8_t(-128)), -128.0);
EXPECT_THROW(
to<float>(std::numeric_limits<uint64_t>::max()), std::range_error);
EXPECT_THROW(
to<float>(std::numeric_limits<int64_t>::max()), std::range_error);
EXPECT_THROW(
to<float>(std::numeric_limits<int64_t>::min() + 1), std::range_error);
#if FOLLY_HAVE_INT128_T
EXPECT_THROW(
to<double>(std::numeric_limits<unsigned __int128>::max()),
std::range_error);
EXPECT_THROW(
to<double>(std::numeric_limits<__int128>::max()), std::range_error);
EXPECT_THROW(
to<double>(std::numeric_limits<__int128>::min() + 1), std::range_error);
#endif
}
TEST(Conv, NewUint64ToString) { TEST(Conv, NewUint64ToString) {
char buf[21]; char buf[21];
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment