From: Marcus Holland-Moritz <mhx@fb.com>
Date: Thu, 23 Jun 2016 03:28:36 +0000 (-0700)
Subject: Fix undefined behaviour in float<->int conversion
X-Git-Tag: 2016.07.26~128
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=47868e4911bee74582d605195525bafa889252d9;p=folly.git

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 79276ca4..450896a6 100644
--- 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 790742bc..a14eeb28 100644
--- 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];