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

@@ -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

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"

include/gsl/narrow

@@ -21,6 +21,32 @@
 #include <exception> // for std::exception
 namespace gsl
 {
+namespace details
+{
+    template <class T, class U>
+    constexpr bool static_cast_is_defined(U u, /*is_floating_point_to_integral=*/std::true_type)
+    {
+        if (std::is_same<typename std::remove_cv<T>::type, bool>::value) { return true; }
+
+        U upper_bound{1};
+        for (int i = 0; i < std::numeric_limits<T>::digits; i++)
+        {
+            upper_bound *= std::numeric_limits<T>::radix;
+        }
+
+        if (u >= U{}) { return u < upper_bound; }
+        if (!std::is_signed<T>::value) { return u > U{-1}; }
+
+        return u + upper_bound > U{-1};
+    }
+
+    template <class T, class U>
+    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 <class T, class U, typename std::enable_if<std::is_arithmetic<T>::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<T>::value != std::is_signed<U>::value);
 
+    using is_floating_point_to_integral =
+        std::integral_constant<bool,
+                               std::is_integral<T>::value && std::is_floating_point<U>::value>;
+    if (!details::static_cast_is_defined<T, U>(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<T>(
-        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<T>(u);
 
 #if defined(__clang__) || defined(__GNUC__)
 #pragma GCC diagnostic push

include/gsl/util

@@ -178,7 +178,14 @@ GSL_NODISCARD auto finally(F&& f) noexcept
 template <class T, class U>
 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<T>(std::forward<U>(u));
+#if defined(__clang__) || defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
 }
 
 //

tests/utils_tests.cpp

@@ -17,9 +17,10 @@
 #include <gtest/gtest.h>
 
 #include <algorithm> // for move
+#include <cmath>     // for ldexp
 #include <complex>
 #include <cstddef>     // for std::ptrdiff_t
-#include <cstdint>     // for uint32_t, int32_t
+#include <cstdint>     // for int32_t, int64_t, uint32_t, uint64_t
 #include <functional>  // for reference_wrapper, _Bind_helper<>::type
 #include <gsl/narrow>  // for narrow, narrowing_error
 #include <gsl/util>    // 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<unsigned>(-0.5, std::true_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<unsigned>(-1.0, std::true_type{}));
+
+    const double uint32_upper_bound = std::ldexp(1.0, std::numeric_limits<uint32_t>::digits);
+    EXPECT_TRUE(details::static_cast_is_defined<uint32_t>(std::nextafter(uint32_upper_bound, 0.0),
+                                                          std::true_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<uint32_t>(uint32_upper_bound, std::true_type{}));
+
+    const double int32_lower_bound = -std::ldexp(1.0, std::numeric_limits<int32_t>::digits);
+    EXPECT_TRUE(
+        details::static_cast_is_defined<int32_t>(int32_lower_bound - 0.5, std::true_type{}));
+    EXPECT_FALSE(
+        details::static_cast_is_defined<int32_t>(int32_lower_bound - 1.0, std::true_type{}));
+
+    const double int32_upper_bound = std::ldexp(1.0, std::numeric_limits<int32_t>::digits);
+    EXPECT_TRUE(details::static_cast_is_defined<int32_t>(std::nextafter(int32_upper_bound, 0.0),
+                                                         std::true_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<int32_t>(int32_upper_bound, std::true_type{}));
+
+    const float int32_min = static_cast<float>((std::numeric_limits<int32_t>::min)());
+    const double int64_min = static_cast<double>((std::numeric_limits<int64_t>::min)());
+    EXPECT_TRUE(details::static_cast_is_defined<int32_t>(int32_min, std::true_type{}));
+    EXPECT_TRUE(details::static_cast_is_defined<int64_t>(int64_min, std::true_type{}));
+
+    EXPECT_TRUE(details::static_cast_is_defined<const bool>(-1.0, std::true_type{}));
+    EXPECT_TRUE(details::static_cast_is_defined<int>(0, std::false_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<int>(std::numeric_limits<double>::infinity(),
+                                                      std::true_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<int>(-std::numeric_limits<double>::infinity(),
+                                                      std::true_type{}));
+    EXPECT_FALSE(details::static_cast_is_defined<int>(std::numeric_limits<double>::quiet_NaN(),
+                                                      std::true_type{}));
+}
+
+TEST(utils_tests, narrow_exact_signed_minimum)
+{
+    EXPECT_NO_THROW({
+        const auto value =
+            narrow<int32_t>(static_cast<float>((std::numeric_limits<int32_t>::min)()));
+        EXPECT_EQ(value, (std::numeric_limits<int32_t>::min)());
+    });
+
+    EXPECT_NO_THROW({
+        const auto value =
+            narrow<int64_t>(static_cast<double>((std::numeric_limits<int64_t>::min)()));
+        EXPECT_EQ(value, (std::numeric_limits<int64_t>::min)());
+    });
+}
+
 TEST(utils_tests, narrow)
 {
     int n = 120;
@@ -161,5 +213,24 @@ TEST(utils_tests, narrow)
     EXPECT_THROW(narrow<std::complex<float>>(std::complex<double>(4.2)), narrowing_error);
 
     EXPECT_TRUE(narrow<int>(float(1)) == 1);
+    EXPECT_TRUE(narrow<bool>(0.0) == false);
+    EXPECT_TRUE(narrow<bool>(1.0) == true);
+    EXPECT_THROW(narrow<bool>(2.0), narrowing_error);
+    EXPECT_THROW(narrow<unsigned char>(256.), narrowing_error);
+    EXPECT_THROW(narrow<unsigned char>(-0.5), narrowing_error);
+    EXPECT_THROW(narrow<unsigned char>(-1.0), narrowing_error);
+    EXPECT_THROW(narrow<int>((std::numeric_limits<float>::max)()), narrowing_error);
+    EXPECT_THROW(narrow<int>((std::numeric_limits<float>::lowest)()), narrowing_error);
+    EXPECT_THROW(narrow<int>(std::numeric_limits<float>::infinity()), narrowing_error);
+    EXPECT_THROW(narrow<int>(std::numeric_limits<float>::quiet_NaN()), narrowing_error);
+
+    const double int32_lower_bound = -std::ldexp(1.0, std::numeric_limits<int32_t>::digits);
+    EXPECT_TRUE(narrow<int32_t>(int32_lower_bound) == std::numeric_limits<int32_t>::min());
+    EXPECT_THROW(narrow<int32_t>(int32_lower_bound - 0.5), narrowing_error);
+
+    const double int64_upper_bound = std::ldexp(1.0, std::numeric_limits<int64_t>::digits);
+    const double uint64_upper_bound = std::ldexp(1.0, std::numeric_limits<uint64_t>::digits);
+    EXPECT_THROW(narrow<int64_t>(int64_upper_bound), narrowing_error);
+    EXPECT_THROW(narrow<uint64_t>(uint64_upper_bound), narrowing_error);
 }
 #endif // GSL_KERNEL_MODE