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

diff --git a/folly/Conv.h b/folly/Conv.h
index 79276ca4c4042f7c47ce655d8ffd40ee7f633e7b..450896a65062f9e09cce0055e0b89dfdcea004ff 100644 (file)
--- a/folly/Conv.h
+++ b/folly/Conv.h
@@ -1160,6 +1160,52 @@ parseTo(StringPiece src, Tgt& out) {
  * 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
  * result must be convertible back to the source type without loss of
@@ -1174,19 +1220,23 @@ typename std::enable_if<
   (std::is_floating_point<Src>::value && std::is_integral<Tgt>::value),
   Tgt>::type
 to(const Src & value) {
-  Tgt result = Tgt(value);
-  auto witness = static_cast<Src>(result);
-  if (value != witness) {
-    throw std::range_error(
-      to<std::string>("to<>: loss of precision when converting ", value,
+  if (detail::checkConversion<Tgt>(value)) {
+    Tgt result = Tgt(value);
+    if (detail::checkConversion<Src>(result)) {
+      auto witness = static_cast<Src>(result);
+      if (value == witness) {
+        return result;
+      }
+    }
+  }
+  throw std::range_error(
+    to<std::string>("to<>: loss of precision when converting ", value,
 #ifdef FOLLY_HAS_RTTI
-                      " to type ", typeid(Tgt).name()
+                    " to type ", typeid(Tgt).name()
 #else
-                      " to other type"
+                    " to other type"
 #endif
-                      ).c_str());
-  }
-  return result;
+                    ).c_str());
 }
 
 /*******************************************************************************

diff --git a/folly/test/ConvTest.cpp b/folly/test/ConvTest.cpp
index 790742bc4b152ee539a74f523633517c3cb66e82..a14eeb2801ef510f3ef4527f825347128e59f698 100644 (file)
--- a/folly/test/ConvTest.cpp
+++ b/folly/test/ConvTest.cpp
@@ -792,6 +792,37 @@ TEST(Conv, StringToBool) {
   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) {
   char buf[21];