Fix float to int conversion in base/numerics

This addresses the bug where range checks are incorrect when
converting from a floating point value to an integral value of
higher precision but lower width. It also reverts a previous
attempted fix that eliminated false negatives but introduced
false positives.

BUG=522989

Review URL: https://codereview.chromium.org/1338553003

Cr-Commit-Position: refs/heads/master@{#348706}

2 files changed, 173 insertions, 8 deletions

diff --git a/base/numerics/safe_conversions_impl.h b/base/numerics/safe_conversions_impl.h
index 4157067..f4bc916 100644
--- a/base/numerics/safe_conversions_impl.h
+++ b/base/numerics/safe_conversions_impl.h
@@ -108,6 +108,55 @@ inline RangeConstraint GetRangeConstraint(bool is_in_upper_bound,
                             (is_in_lower_bound ? 0 : RANGE_UNDERFLOW));
 }
 
+// The following helper template addresses a corner case in range checks for
+// conversion from a floating-point type to an integral type of smaller range
+// but larger precision (e.g. float -> unsigned). The problem is as follows:
+//   1. Integral maximum is always one less than a power of two, so it must be
+//      truncated to fit the mantissa of the floating point. The direction of
+//      rounding is implementation defined, but by default it's always IEEE
+//      floats, which round to nearest and thus result in a value of larger
+//      magnitude than the integral value.
+//      Example: float f = UINT_MAX; // f is 4294967296f but UINT_MAX
+//                                   // is 4294967295u.
+//   2. If the floating point value is equal to the promoted integral maximum
+//      value, a range check will erroneously pass.
+//      Example: (4294967296f <= 4294967295u) // This is true due to a precision
+//                                            // loss in rounding up to float.
+//   3. When the floating point value is then converted to an integral, the
+//      resulting value is out of range for the target integral type and
+//      thus is implementation defined.
+//      Example: unsigned u = (float)INT_MAX; // u will typically overflow to 0.
+// To fix this bug we manually truncate the maximum value when the destination
+// type is an integral of larger precision than the source floating-point type,
+// such that the resulting maximum is represented exactly as a floating point.
+template <typename Dst, typename Src>
+struct NarrowingRange {
+  typedef typename std::numeric_limits<Src> SrcLimits;
+  typedef typename std::numeric_limits<Dst> DstLimits;
+
+  static Dst max() {
+    // The following logic avoids warnings where the max function is
+    // instantiated with invalid values for a bit shift (even though
+    // such a function can never be called).
+    static const int shift =
+        (MaxExponent<Src>::value > MaxExponent<Dst>::value &&
+         SrcLimits::digits < DstLimits::digits && SrcLimits::is_iec559 &&
+         DstLimits::is_integer)
+            ? (DstLimits::digits - SrcLimits::digits)
+            : 0;
+
+    // We use UINTMAX_C below to avoid compiler warnings about shifting floating
+    // points. Since it's a compile time calculation, it shouldn't have any
+    // performance impact.
+    return DstLimits::max() - static_cast<Dst>((UINTMAX_C(1) << shift) - 1);
+  }
+
+  static Dst min() {
+    return std::numeric_limits<Dst>::is_iec559 ? -DstLimits::max()
+                                               : DstLimits::min();
+  }
+};
+
 template <
     typename Dst,
     typename Src,
@@ -147,11 +196,8 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
                                       INTEGER_REPRESENTATION_SIGNED,
                                       NUMERIC_RANGE_NOT_CONTAINED> {
   static RangeConstraint Check(Src value) {
-    return std::numeric_limits<Dst>::is_iec559
-               ? GetRangeConstraint((value < std::numeric_limits<Dst>::max()),
-                                    (value > -std::numeric_limits<Dst>::max()))
-               : GetRangeConstraint((value < std::numeric_limits<Dst>::max()),
-                                    (value > std::numeric_limits<Dst>::min()));
+    return GetRangeConstraint((value <= NarrowingRange<Dst, Src>::max()),
+                              (value >= NarrowingRange<Dst, Src>::min()));
   }
 };
 
@@ -163,7 +209,7 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
                                       INTEGER_REPRESENTATION_UNSIGNED,
                                       NUMERIC_RANGE_NOT_CONTAINED> {
   static RangeConstraint Check(Src value) {
-    return GetRangeConstraint(value < std::numeric_limits<Dst>::max(), true);
+    return GetRangeConstraint(value <= NarrowingRange<Dst, Src>::max(), true);
   }
 };
 
@@ -178,7 +224,7 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
     return sizeof(Dst) > sizeof(Src)
                ? RANGE_VALID
                : GetRangeConstraint(
-                     value < static_cast<Src>(std::numeric_limits<Dst>::max()),
+                     value <= static_cast<Src>(NarrowingRange<Dst, Src>::max()),
                      true);
   }
 };
@@ -195,7 +241,7 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
     return (MaxExponent<Dst>::value >= MaxExponent<Src>::value)
                ? GetRangeConstraint(true, value >= static_cast<Src>(0))
                : GetRangeConstraint(
-                     value < static_cast<Src>(std::numeric_limits<Dst>::max()),
+                     value <= static_cast<Src>(NarrowingRange<Dst, Src>::max()),
                      value >= static_cast<Src>(0));
   }
 };
diff --git a/base/numerics/safe_numerics_unittest.cc b/base/numerics/safe_numerics_unittest.cc
index 6f9a966..bad0f57 100644
--- a/base/numerics/safe_numerics_unittest.cc
+++ b/base/numerics/safe_numerics_unittest.cc
@@ -18,6 +18,7 @@
 using std::numeric_limits;
 using base::CheckedNumeric;
 using base::checked_cast;
+using base::IsValueInRangeForNumericType;
 using base::SizeT;
 using base::StrictNumeric;
 using base::saturated_cast;
@@ -36,6 +37,26 @@ using base::enable_if;
 #pragma warning(disable:4756)
 #endif
 
+// This is a helper function for finding the maximum value in Src that can be
+// wholy represented as the destination floating-point type.
+template <typename Dst, typename Src>
+Dst GetMaxConvertibleToFloat() {
+  typedef numeric_limits<Dst> DstLimits;
+  typedef numeric_limits<Src> SrcLimits;
+  static_assert(SrcLimits::is_specialized, "Source must be numeric.");
+  static_assert(DstLimits::is_specialized, "Destination must be numeric.");
+  CHECK(DstLimits::is_iec559);
+
+  if (SrcLimits::digits <= DstLimits::digits &&
+      MaxExponent<Src>::value <= MaxExponent<Dst>::value)
+    return SrcLimits::max();
+  Src max = SrcLimits::max() / 2 + (SrcLimits::is_integer ? 1 : 0);
+  while (max != static_cast<Src>(static_cast<Dst>(max))) {
+    max /= 2;
+  }
+  return static_cast<Dst>(max);
+}
+
 // Helper macros to wrap displaying the conversion types and line numbers.
 #define TEST_EXPECTED_VALIDITY(expected, actual)                           \
   EXPECT_EQ(expected, CheckedNumeric<Dst>(actual).validity())              \
@@ -370,6 +391,18 @@ struct TestNumericConversion<Dst, Src, SIGN_PRESERVING_NARROW> {
       TEST_EXPECTED_RANGE(RANGE_OVERFLOW, SrcLimits::infinity());
       TEST_EXPECTED_RANGE(RANGE_UNDERFLOW, SrcLimits::infinity() * -1);
       TEST_EXPECTED_RANGE(RANGE_INVALID, SrcLimits::quiet_NaN());
+      if (DstLimits::is_integer) {
+        if (SrcLimits::digits < DstLimits::digits) {
+          TEST_EXPECTED_RANGE(RANGE_OVERFLOW,
+                              static_cast<Src>(DstLimits::max()));
+        } else {
+          TEST_EXPECTED_RANGE(RANGE_VALID, static_cast<Src>(DstLimits::max()));
+        }
+        TEST_EXPECTED_RANGE(
+            RANGE_VALID,
+            static_cast<Src>(GetMaxConvertibleToFloat<Src, Dst>()));
+        TEST_EXPECTED_RANGE(RANGE_VALID, static_cast<Src>(DstLimits::min()));
+      }
     } else if (SrcLimits::is_signed) {
       TEST_EXPECTED_VALUE(-1, checked_dst - static_cast<Src>(1));
       TEST_EXPECTED_RANGE(RANGE_UNDERFLOW, SrcLimits::min());
@@ -428,6 +461,18 @@ struct TestNumericConversion<Dst, Src, SIGN_TO_UNSIGN_NARROW> {
       TEST_EXPECTED_RANGE(RANGE_OVERFLOW, SrcLimits::infinity());
       TEST_EXPECTED_RANGE(RANGE_UNDERFLOW, SrcLimits::infinity() * -1);
       TEST_EXPECTED_RANGE(RANGE_INVALID, SrcLimits::quiet_NaN());
+      if (DstLimits::is_integer) {
+        if (SrcLimits::digits < DstLimits::digits) {
+          TEST_EXPECTED_RANGE(RANGE_OVERFLOW,
+                              static_cast<Src>(DstLimits::max()));
+        } else {
+          TEST_EXPECTED_RANGE(RANGE_VALID, static_cast<Src>(DstLimits::max()));
+        }
+        TEST_EXPECTED_RANGE(
+            RANGE_VALID,
+            static_cast<Src>(GetMaxConvertibleToFloat<Src, Dst>()));
+        TEST_EXPECTED_RANGE(RANGE_VALID, static_cast<Src>(DstLimits::min()));
+      }
     } else {
       TEST_EXPECTED_RANGE(RANGE_UNDERFLOW, SrcLimits::min());
     }
@@ -600,3 +645,77 @@ TEST(SafeNumerics, CastTests) {
   EXPECT_EQ(numeric_limits<int>::max(), saturated_cast<int>(double_large_int));
 }
 
+TEST(SafeNumerics, IsValueInRangeForNumericType) {
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(0));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(2));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint32_t>(-1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(0xffffffffu));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(UINT64_C(0xffffffff)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint32_t>(UINT64_C(0x100000000)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint32_t>(UINT64_C(0x100000001)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint32_t>(
+      std::numeric_limits<int32_t>::min()));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint32_t>(
+      std::numeric_limits<int64_t>::min()));
+
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(0));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(2));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(-1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(0x7fffffff));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(0x7fffffffu));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(0x80000000u));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(0xffffffffu));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(INT64_C(0x80000000)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(INT64_C(0xffffffff)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(INT64_C(0x100000000)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(
+      std::numeric_limits<int32_t>::min()));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int32_t>(
+      implicit_cast<int64_t>(std::numeric_limits<int32_t>::min())));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(
+      implicit_cast<int64_t>(std::numeric_limits<int32_t>::min()) - 1));
+  EXPECT_FALSE(IsValueInRangeForNumericType<int32_t>(
+      std::numeric_limits<int64_t>::min()));
+
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(0));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(2));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint64_t>(-1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(0xffffffffu));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(UINT64_C(0xffffffff)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(UINT64_C(0x100000000)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<uint64_t>(UINT64_C(0x100000001)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint64_t>(
+      std::numeric_limits<int32_t>::min()));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint64_t>(INT64_C(-1)));
+  EXPECT_FALSE(IsValueInRangeForNumericType<uint64_t>(
+      std::numeric_limits<int64_t>::min()));
+
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(0));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(2));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(-1));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(0x7fffffff));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(0x7fffffffu));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(0x80000000u));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(0xffffffffu));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(INT64_C(0x80000000)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(INT64_C(0xffffffff)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(INT64_C(0x100000000)));
+  EXPECT_TRUE(
+      IsValueInRangeForNumericType<int64_t>(INT64_C(0x7fffffffffffffff)));
+  EXPECT_TRUE(
+      IsValueInRangeForNumericType<int64_t>(UINT64_C(0x7fffffffffffffff)));
+  EXPECT_FALSE(
+      IsValueInRangeForNumericType<int64_t>(UINT64_C(0x8000000000000000)));
+  EXPECT_FALSE(
+      IsValueInRangeForNumericType<int64_t>(UINT64_C(0xffffffffffffffff)));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(
+      std::numeric_limits<int32_t>::min()));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(
+      implicit_cast<int64_t>(std::numeric_limits<int32_t>::min())));
+  EXPECT_TRUE(IsValueInRangeForNumericType<int64_t>(
+      std::numeric_limits<int64_t>::min()));
+}