Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(513)

Unified Diff: base/numerics/safe_conversions_impl.h

Issue 1338553003: Fix float to int conversion in base/numerics (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: more tests Created 5 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | base/numerics/safe_numerics_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: base/numerics/safe_conversions_impl.h
diff --git a/base/numerics/safe_conversions_impl.h b/base/numerics/safe_conversions_impl.h
index 41570671601bbc5264bed1990e905ecd9bc46f45..f4bc9161a0d3500ac345b14d9f1b4e7935e64bb5 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));
}
};
« no previous file with comments | « no previous file | base/numerics/safe_numerics_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698