From 48712168b8222d4e48371fa1bd803d83fd90334d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Thu, 28 Jan 2021 12:45:50 +0100 Subject: [PATCH 1/3] MathUtil: Add SaturatingCast to cast floats more safely --- Source/Core/Common/MathUtil.h | 43 ++++++++++++++++++++++++ Source/UnitTests/Common/MathUtilTest.cpp | 42 +++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/Source/Core/Common/MathUtil.h b/Source/Core/Common/MathUtil.h index 18a0c305ce..2aa2b406ab 100644 --- a/Source/Core/Common/MathUtil.h +++ b/Source/Core/Common/MathUtil.h @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -30,6 +31,48 @@ constexpr auto Lerp(const T& x, const T& y, const F& a) -> decltype(x + (y - x) return x + (y - x) * a; } +// Casts the specified value to a Dest. The value will be clamped to fit in the destination type. +// Warning: The result of SaturatingCast(NaN) is undefined. +template +constexpr Dest SaturatingCast(T value) +{ + static_assert(std::is_integral()); + + constexpr Dest lo = std::numeric_limits::lowest(); + constexpr Dest hi = std::numeric_limits::max(); + + // T being a signed integer and Dest unsigned is a problematic case because the value will + // be converted into an unsigned integer, and u32(...) < 0 is always false. + if constexpr (std::is_integral() && std::is_signed() && std::is_unsigned()) + { + static_assert(lo == 0); + if (value < 0) + return lo; + // Now that we got rid of negative values, we can safely cast value to an unsigned T + // since unsigned T can represent any positive value signed T could represent. + // The compiler will then promote the LHS or the RHS if necessary. + if (std::make_unsigned_t(value) > hi) + return hi; + } + else if constexpr (std::is_integral() && std::is_unsigned() && std::is_signed()) + { + // value and hi will never be negative, and hi is representable as an unsigned Dest. + if (value > std::make_unsigned_t(hi)) + return hi; + } + else + { + // Do not use std::clamp or a similar function here to avoid overflow. + // For example, if Dest = s64 and T = int, we want integer promotion to convert value to a s64 + // instead of changing lo or hi into an int. + if (value < lo) + return lo; + if (value > hi) + return hi; + } + return static_cast(value); +} + template constexpr bool IsPow2(T imm) { diff --git a/Source/UnitTests/Common/MathUtilTest.cpp b/Source/UnitTests/Common/MathUtilTest.cpp index c0609bbb92..e31777d16d 100644 --- a/Source/UnitTests/Common/MathUtilTest.cpp +++ b/Source/UnitTests/Common/MathUtilTest.cpp @@ -26,3 +26,45 @@ TEST(MathUtil, NextPowerOf2) EXPECT_EQ(8U, MathUtil::NextPowerOf2(6)); EXPECT_EQ(0x40000000U, MathUtil::NextPowerOf2(0x23456789)); } + +TEST(MathUtil, SaturatingCast) +{ + // Cast from an integer type to a smaller type + EXPECT_EQ(255u, (MathUtil::SaturatingCast(1000))); + EXPECT_EQ(255u, (MathUtil::SaturatingCast(1000u))); + EXPECT_EQ(255u, (MathUtil::SaturatingCast(1000))); + + // Cast from a signed integer type + EXPECT_EQ(0u, (MathUtil::SaturatingCast(-1))); + EXPECT_EQ(0u, (MathUtil::SaturatingCast(-1000))); + EXPECT_EQ(0u, (MathUtil::SaturatingCast(-1))); + EXPECT_EQ(-1000, (MathUtil::SaturatingCast(-1000))); + EXPECT_EQ(-1000, (MathUtil::SaturatingCast(-1000))); + EXPECT_EQ(-1000, (MathUtil::SaturatingCast(-1000))); + + // Cast from an unsigned integer type to a smaller integer type + EXPECT_EQ(0x7fff, (MathUtil::SaturatingCast(0xffffffffu))); + EXPECT_EQ(0x7fffffff, (MathUtil::SaturatingCast(0xffffffffu))); + + // Cast from a floating point type to an integer type + EXPECT_EQ(255u, MathUtil::SaturatingCast(1234.0)); + EXPECT_EQ(0u, MathUtil::SaturatingCast(-1234.0)); + EXPECT_EQ(127, MathUtil::SaturatingCast(5678.0)); + EXPECT_EQ(-128, MathUtil::SaturatingCast(-5678.0)); + EXPECT_EQ(65535u, MathUtil::SaturatingCast(999999.0)); + + // Negative zero + EXPECT_EQ(0u, MathUtil::SaturatingCast(0.0)); + EXPECT_EQ(0u, MathUtil::SaturatingCast(-0.0)); + EXPECT_EQ(0, MathUtil::SaturatingCast(0.0)); + EXPECT_EQ(0, MathUtil::SaturatingCast(-0.0)); + + // Edge cases + EXPECT_EQ(std::numeric_limits::max(), + MathUtil::SaturatingCast(std::numeric_limits::infinity())); + EXPECT_EQ(std::numeric_limits::min(), + MathUtil::SaturatingCast(-std::numeric_limits::infinity())); + // 16777217 = 2^24 + 1 is the first integer that cannot be represented correctly with a f32. + EXPECT_EQ(16777216, MathUtil::SaturatingCast(float(16777216))); + EXPECT_EQ(16777216, MathUtil::SaturatingCast(float(16777217))); +} From 8d21fa56a1133529273d57df06888e42bb63fde7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Thu, 28 Jan 2021 12:49:28 +0100 Subject: [PATCH 2/3] UnitTests: Use MathUtil::SaturatingCast to avoid UB [conv.fpint]/1: > A prvalue of a floating-point type can be converted to a prvalue of > an integer type. The conversion truncates; that is, the fractional > part is discarded. The behavior is undefined if the truncated value > cannot be represented in the destination type. --- .../VideoCommon/VertexLoaderTest.cpp | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Source/UnitTests/VideoCommon/VertexLoaderTest.cpp b/Source/UnitTests/VideoCommon/VertexLoaderTest.cpp index 4bad4a35a7..7f9dbdec84 100644 --- a/Source/UnitTests/VideoCommon/VertexLoaderTest.cpp +++ b/Source/UnitTests/VideoCommon/VertexLoaderTest.cpp @@ -12,6 +12,7 @@ #include "Common/BitUtils.h" #include "Common/Common.h" +#include "Common/MathUtil.h" #include "VideoCommon/CPMemory.h" #include "VideoCommon/DataReader.h" #include "VideoCommon/OpcodeDecoding.h" @@ -148,7 +149,7 @@ TEST_P(VertexLoaderParamTest, PositionAll) -0x8000, -0x80, -1, - -0, + -0.0, 0, 1, 123, @@ -180,16 +181,16 @@ TEST_P(VertexLoaderParamTest, PositionAll) switch (format) { case ComponentFormat::UByte: - Input((u8)value); + Input(MathUtil::SaturatingCast(value)); break; case ComponentFormat::Byte: - Input((s8)value); + Input(MathUtil::SaturatingCast(value)); break; case ComponentFormat::UShort: - Input((u16)value); + Input(MathUtil::SaturatingCast(value)); break; case ComponentFormat::Short: - Input((s16)value); + Input(MathUtil::SaturatingCast(value)); break; case ComponentFormat::Float: Input(value); @@ -206,20 +207,20 @@ TEST_P(VertexLoaderParamTest, PositionAll) switch (format) { case ComponentFormat::UByte: - f = (u8)*iter++; - g = (u8)*iter++; + f = MathUtil::SaturatingCast(*iter++); + g = MathUtil::SaturatingCast(*iter++); break; case ComponentFormat::Byte: - f = (s8)*iter++; - g = (s8)*iter++; + f = MathUtil::SaturatingCast(*iter++); + g = MathUtil::SaturatingCast(*iter++); break; case ComponentFormat::UShort: - f = (u16)*iter++; - g = (u16)*iter++; + f = MathUtil::SaturatingCast(*iter++); + g = MathUtil::SaturatingCast(*iter++); break; case ComponentFormat::Short: - f = (s16)*iter++; - g = (s16)*iter++; + f = MathUtil::SaturatingCast(*iter++); + g = MathUtil::SaturatingCast(*iter++); break; case ComponentFormat::Float: f = *iter++; From 1a9e72c9bbed1842621969f035dec47ac75d9db4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Thu, 28 Jan 2021 17:28:21 +0100 Subject: [PATCH 3/3] DiscIO: Use MathUtil::SaturatingCast --- Source/Core/DiscIO/WIACompression.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Source/Core/DiscIO/WIACompression.cpp b/Source/Core/DiscIO/WIACompression.cpp index 5b5da07a37..17231f63b3 100644 --- a/Source/Core/DiscIO/WIACompression.cpp +++ b/Source/Core/DiscIO/WIACompression.cpp @@ -19,6 +19,7 @@ #include "Common/Assert.h" #include "Common/CommonTypes.h" +#include "Common/MathUtil.h" #include "Common/Swap.h" #include "DiscIO/LaggedFibonacciGenerator.h" @@ -166,18 +167,13 @@ bool Bzip2Decompressor::Decompress(const DecompressionBuffer& in, DecompressionB m_started = true; } - constexpr auto clamped_cast = [](size_t x) { - return static_cast( - std::min(std::numeric_limits().max(), x)); - }; - char* const in_ptr = reinterpret_cast(const_cast(in.data.data() + *in_bytes_read)); m_stream.next_in = in_ptr; - m_stream.avail_in = clamped_cast(in.bytes_written - *in_bytes_read); + m_stream.avail_in = MathUtil::SaturatingCast(in.bytes_written - *in_bytes_read); char* const out_ptr = reinterpret_cast(out->data.data() + out->bytes_written); m_stream.next_out = out_ptr; - m_stream.avail_out = clamped_cast(out->data.size() - out->bytes_written); + m_stream.avail_out = MathUtil::SaturatingCast(out->data.size() - out->bytes_written); const int result = BZ2_bzDecompress(&m_stream);