From 08687fb42af451fae5c6aa690163f5c95432fb50 Mon Sep 17 00:00:00 2001 From: Carson Radtke Date: Tue, 25 Aug 2026 11:29:25 -0600 Subject: [PATCH] Fix floating-to-integral narrow undefined behavior (#1266) * Fix floating-to-integral narrow undefined behavior * Format narrow conversion changes * Suppress float-equal warning in narrow_cast --- .github/workflows/compilers.yml | 22 ++++++++++ CMakePresets.json | 45 ++++++++++++++++++++ include/gsl/narrow | 48 ++++++++++++++++------ include/gsl/util | 7 ++++ tests/utils_tests.cpp | 73 ++++++++++++++++++++++++++++++++- 5 files changed, 182 insertions(+), 13 deletions(-) diff --git a/.github/workflows/compilers.yml b/.github/workflows/compilers.yml index fd56e8e..e901391 100644 --- a/.github/workflows/compilers.yml +++ b/.github/workflows/compilers.yml @@ -58,6 +58,16 @@ jobs: with: cmake_preset: clang-${{ matrix.cxx_version }}-${{ matrix.build_type == 'Debug' && 'debug' || 'release' }} + linux-sanitizers: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Run CMake with AddressSanitizer and UndefinedBehaviorSanitizer + uses: ./.github/workflows/cmake + with: + cmake_preset: clang-20-debug-asan-ubsan + xcode: strategy: matrix: @@ -108,3 +118,15 @@ jobs: extra_cmake_configure_args: ${{ matrix.generator_override }} extra_cmake_build_args: ${{ matrix.toolset == 'ClangCL' && format('--config {0}', matrix.build_type) || '' }} extra_ctest_args: ${{ matrix.toolset == 'ClangCL' && format('-C {0}', matrix.build_type) || '' }} + + windows-sanitizer: + runs-on: windows-latest + steps: + - uses: actions/checkout@v6 + - uses: microsoft/setup-msbuild@v3 + - uses: ilammy/msvc-dev-cmd@v1 + + - name: Run CMake with AddressSanitizer + uses: ./.github/workflows/cmake + with: + cmake_preset: msvc-20-debug-asan diff --git a/CMakePresets.json b/CMakePresets.json index d5db429..da19ca5 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -88,6 +88,14 @@ "GSL_CXX_STANDARD": "20" } }, + { + "name": "msvc-20-debug-asan", + "displayName": "MSVC C++20 Debug with AddressSanitizer", + "inherits": "msvc-20-debug", + "cacheVariables": { + "CMAKE_CXX_FLAGS": "/fsanitize=address" + } + }, { "name": "msvc-20-release", "displayName": "MSVC C++20 Release", @@ -232,6 +240,20 @@ "GSL_CXX_STANDARD": "20" } }, + { + "name": "clang-20-debug-asan-ubsan", + "displayName": "Clang C++20 Debug with AddressSanitizer and UndefinedBehaviorSanitizer", + "inherits": "clang-20-debug", + "condition": { + "type": "equals", + "lhs": "${hostSystemName}", + "rhs": "Linux" + }, + "cacheVariables": { + "CMAKE_CXX_FLAGS": "-fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer", + "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=address,undefined" + } + }, { "name": "clang-20-release", "displayName": "Clang C++20 Release", @@ -281,6 +303,10 @@ "name": "msvc-20-debug", "configurePreset": "msvc-20-debug" }, + { + "name": "msvc-20-debug-asan", + "configurePreset": "msvc-20-debug-asan" + }, { "name": "msvc-20-release", "configurePreset": "msvc-20-release" @@ -345,6 +371,10 @@ "name": "clang-20-debug", "configurePreset": "clang-20-debug" }, + { + "name": "clang-20-debug-asan-ubsan", + "configurePreset": "clang-20-debug-asan-ubsan" + }, { "name": "clang-20-release", "configurePreset": "clang-20-release" @@ -379,6 +409,13 @@ "name": "msvc-20-debug", "configurePreset": "msvc-20-debug" }, + { + "name": "msvc-20-debug-asan", + "configurePreset": "msvc-20-debug-asan", + "environment": { + "ASAN_OPTIONS": "halt_on_error=1" + } + }, { "name": "msvc-20-release", "configurePreset": "msvc-20-release" @@ -443,6 +480,14 @@ "name": "clang-20-debug", "configurePreset": "clang-20-debug" }, + { + "name": "clang-20-debug-asan-ubsan", + "configurePreset": "clang-20-debug-asan-ubsan", + "environment": { + "ASAN_OPTIONS": "detect_leaks=1:halt_on_error=1", + "UBSAN_OPTIONS": "print_stacktrace=1:halt_on_error=1" + } + }, { "name": "clang-20-release", "configurePreset": "clang-20-release" diff --git a/include/gsl/narrow b/include/gsl/narrow index 5c65717..7d30959 100644 --- a/include/gsl/narrow +++ b/include/gsl/narrow @@ -21,6 +21,32 @@ #include // for std::exception namespace gsl { +namespace details +{ + template + constexpr bool static_cast_is_defined(U u, /*is_floating_point_to_integral=*/std::true_type) + { + if (std::is_same::type, bool>::value) { return true; } + + U upper_bound{1}; + for (int i = 0; i < std::numeric_limits::digits; i++) + { + upper_bound *= std::numeric_limits::radix; + } + + if (u >= U{}) { return u < upper_bound; } + if (!std::is_signed::value) { return u > U{-1}; } + + return u + upper_bound > U{-1}; + } + + template + constexpr bool static_cast_is_defined(U, /*is_floating_point_to_integral=*/std::false_type) + { + return true; + } +} // namespace details + struct narrowing_error : public std::exception { const char* what() const noexcept override { return "narrowing_error"; } @@ -28,24 +54,22 @@ struct narrowing_error : public std::exception // narrow() : a checked version of narrow_cast() that throws if the cast changed the value template ::value>::type* = nullptr> -GSL_SUPPRESS(type.1) - GSL_SUPPRESS(es.46) // The warning suggests that a floating->unsigned conversion can occur - // in the static_cast below, and that gsl::narrow should be used instead. - // Suppress this warning, since gsl::narrow is defined in terms of - // static_cast - constexpr T narrow(U u) +GSL_SUPPRESS(type.1) constexpr T narrow(U u) { constexpr const bool is_different_signedness = (std::is_signed::value != std::is_signed::value); + using is_floating_point_to_integral = + std::integral_constant::value && std::is_floating_point::value>; + if (!details::static_cast_is_defined(u, is_floating_point_to_integral{})) + { + throw narrowing_error{}; + } + GSL_SUPPRESS(es.103) // don't overflow GSL_SUPPRESS(es.104) // don't underflow - GSL_SUPPRESS(p.2) // don't rely on undefined behavior - const T t = narrow_cast( - u); // While this is technically undefined behavior in some cases (i.e., if the source value - // is of floating-point type and cannot fit into the destination integral type), the - // resultant behavior is benign on the platforms that we target (i.e., no hardware trap - // representations are hit). + const T t = narrow_cast(u); #if defined(__clang__) || defined(__GNUC__) #pragma GCC diagnostic push diff --git a/include/gsl/util b/include/gsl/util index 9c2627e..2ce264d 100644 --- a/include/gsl/util +++ b/include/gsl/util @@ -178,7 +178,14 @@ GSL_NODISCARD auto finally(F&& f) noexcept template GSL_SUPPRESS(type.1) constexpr T narrow_cast(U&& u) noexcept { +#if defined(__clang__) || defined(__GNUC__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wfloat-equal" +#endif return static_cast(std::forward(u)); +#if defined(__clang__) || defined(__GNUC__) +#pragma GCC diagnostic pop +#endif } // diff --git a/tests/utils_tests.cpp b/tests/utils_tests.cpp index cb82ec2..0a79db2 100644 --- a/tests/utils_tests.cpp +++ b/tests/utils_tests.cpp @@ -17,9 +17,10 @@ #include #include // for move +#include // for ldexp #include #include // for std::ptrdiff_t -#include // for uint32_t, int32_t +#include // for int32_t, int64_t, uint32_t, uint64_t #include // for reference_wrapper, _Bind_helper<>::type #include // for narrow, narrowing_error #include // finally, narrow_cast @@ -134,6 +135,57 @@ TEST(utils_tests, narrow_cast) } #ifndef GSL_KERNEL_MODE +TEST(utils_tests, static_cast_is_defined) +{ + EXPECT_TRUE(details::static_cast_is_defined(-0.5, std::true_type{})); + EXPECT_FALSE(details::static_cast_is_defined(-1.0, std::true_type{})); + + const double uint32_upper_bound = std::ldexp(1.0, std::numeric_limits::digits); + EXPECT_TRUE(details::static_cast_is_defined(std::nextafter(uint32_upper_bound, 0.0), + std::true_type{})); + EXPECT_FALSE(details::static_cast_is_defined(uint32_upper_bound, std::true_type{})); + + const double int32_lower_bound = -std::ldexp(1.0, std::numeric_limits::digits); + EXPECT_TRUE( + details::static_cast_is_defined(int32_lower_bound - 0.5, std::true_type{})); + EXPECT_FALSE( + details::static_cast_is_defined(int32_lower_bound - 1.0, std::true_type{})); + + const double int32_upper_bound = std::ldexp(1.0, std::numeric_limits::digits); + EXPECT_TRUE(details::static_cast_is_defined(std::nextafter(int32_upper_bound, 0.0), + std::true_type{})); + EXPECT_FALSE(details::static_cast_is_defined(int32_upper_bound, std::true_type{})); + + const float int32_min = static_cast((std::numeric_limits::min)()); + const double int64_min = static_cast((std::numeric_limits::min)()); + EXPECT_TRUE(details::static_cast_is_defined(int32_min, std::true_type{})); + EXPECT_TRUE(details::static_cast_is_defined(int64_min, std::true_type{})); + + EXPECT_TRUE(details::static_cast_is_defined(-1.0, std::true_type{})); + EXPECT_TRUE(details::static_cast_is_defined(0, std::false_type{})); + EXPECT_FALSE(details::static_cast_is_defined(std::numeric_limits::infinity(), + std::true_type{})); + EXPECT_FALSE(details::static_cast_is_defined(-std::numeric_limits::infinity(), + std::true_type{})); + EXPECT_FALSE(details::static_cast_is_defined(std::numeric_limits::quiet_NaN(), + std::true_type{})); +} + +TEST(utils_tests, narrow_exact_signed_minimum) +{ + EXPECT_NO_THROW({ + const auto value = + narrow(static_cast((std::numeric_limits::min)())); + EXPECT_EQ(value, (std::numeric_limits::min)()); + }); + + EXPECT_NO_THROW({ + const auto value = + narrow(static_cast((std::numeric_limits::min)())); + EXPECT_EQ(value, (std::numeric_limits::min)()); + }); +} + TEST(utils_tests, narrow) { int n = 120; @@ -161,5 +213,24 @@ TEST(utils_tests, narrow) EXPECT_THROW(narrow>(std::complex(4.2)), narrowing_error); EXPECT_TRUE(narrow(float(1)) == 1); + EXPECT_TRUE(narrow(0.0) == false); + EXPECT_TRUE(narrow(1.0) == true); + EXPECT_THROW(narrow(2.0), narrowing_error); + EXPECT_THROW(narrow(256.), narrowing_error); + EXPECT_THROW(narrow(-0.5), narrowing_error); + EXPECT_THROW(narrow(-1.0), narrowing_error); + EXPECT_THROW(narrow((std::numeric_limits::max)()), narrowing_error); + EXPECT_THROW(narrow((std::numeric_limits::lowest)()), narrowing_error); + EXPECT_THROW(narrow(std::numeric_limits::infinity()), narrowing_error); + EXPECT_THROW(narrow(std::numeric_limits::quiet_NaN()), narrowing_error); + + const double int32_lower_bound = -std::ldexp(1.0, std::numeric_limits::digits); + EXPECT_TRUE(narrow(int32_lower_bound) == std::numeric_limits::min()); + EXPECT_THROW(narrow(int32_lower_bound - 0.5), narrowing_error); + + const double int64_upper_bound = std::ldexp(1.0, std::numeric_limits::digits); + const double uint64_upper_bound = std::ldexp(1.0, std::numeric_limits::digits); + EXPECT_THROW(narrow(int64_upper_bound), narrowing_error); + EXPECT_THROW(narrow(uint64_upper_bound), narrowing_error); } #endif // GSL_KERNEL_MODE