remove undefined bahavior (#290)

* change the checked_multiply function to not use undefined behavior to check for potential undefined behavior and wrapping. * update the checked_multiply template to deal with mismatched sign in signed numbers and min val correctly. This involved adding to templates to clear up warnings

remove undefined bahavior (#290)
* change the checked_multiply function to not use undefined behavior to check for potential undefined behavior and wrapping. * update the checked_multiply template to deal with mismatched sign in signed numbers and min val correctly. This involved adding to templates to clear up warnings
Philip Top · Henry Schreiner
1 parent eab92ed9
Showing 2 changed files with 49 additions and 5 deletions
include/CLI/Validators.hpp
tests/HelpersTest.cpp
 #pragma once
-
 // Distributed under the 3-Clause BSD License.  See accompanying
 // file LICENSE or https://github.com/CLIUtils/CLI11 for details.
@@ -9,6 +8,7 @@
 #include <cmath>
 #include <functional>
 #include <iostream>
+#include <limits>
 #include <map>
 #include <memory>
 #include <string>
@@ -512,17 +512,37 @@ auto search(const T &amp;set, const V &amp;val, const std::function&lt;V(V)&gt; &amp;filter_functi
     return {(it != std::end(setref)), it};
 }
+// the following suggestion was made by Nikita Ofitserov(@himikof)
+// done in templates to prevent compiler warnings on negation of unsigned numbers
+
+/// Do a check for overflow on signed numbers
+template <typename T>
+inline typename std::enable_if<std::is_signed<T>::value, T>::type overflowCheck(const T &a, const T &b) {
+    if((a > 0) == (b > 0)) {
+        return ((std::numeric_limits<T>::max)() / (std::abs)(a) < (std::abs)(b));
+    } else {
+        return ((std::numeric_limits<T>::min)() / (std::abs)(a) > -(std::abs)(b));
+    }
+}
+/// Do a check for overflow on unsigned numbers
+template <typename T>
+inline typename std::enable_if<!std::is_signed<T>::value, T>::type overflowCheck(const T &a, const T &b) {
+    return ((std::numeric_limits<T>::max)() / a < b);
+}
+
 /// Performs a *= b; if it doesn't cause integer overflow. Returns false otherwise.
 template <typename T> typename std::enable_if<std::is_integral<T>::value, bool>::type checked_multiply(T &a, T b) {
-    if(a == 0 || b == 0) {
+    if(a == 0 || b == 0 || a == 1 || b == 1) {
         a *= b;
         return true;
     }
-    T c = a * b;
-    if(c / a != b) {
+    if(a == (std::numeric_limits<T>::min)() || b == (std::numeric_limits<T>::min)()) {
         return false;
     }
-    a = c;
+    if(overflowCheck(a, b)) {
+        return false;
+    }
+    a *= b;
     return true;
 }
@@ -423,10 +423,34 @@ TEST(CheckedMultiply, Int) {
     ASSERT_FALSE(CLI::detail::checked_multiply(a, b));
     ASSERT_EQ(a, std::numeric_limits<int>::min());
+    b = std::numeric_limits<int>::min();
+    a = -1;
+    ASSERT_FALSE(CLI::detail::checked_multiply(a, b));
+    ASSERT_EQ(a, -1);
+
     a = std::numeric_limits<int>::min() / 100;
     b = 99;
     ASSERT_TRUE(CLI::detail::checked_multiply(a, b));
     ASSERT_EQ(a, std::numeric_limits<int>::min() / 100 * 99);
+
+    a = std::numeric_limits<int>::min() / 100;
+    b = -101;
+    ASSERT_FALSE(CLI::detail::checked_multiply(a, b));
+    ASSERT_EQ(a, std::numeric_limits<int>::min() / 100);
+    a = 2;
+    b = std::numeric_limits<int>::min() / 2;
+    ASSERT_TRUE(CLI::detail::checked_multiply(a, b));
+    a = std::numeric_limits<int>::min() / 2;
+    b = 2;
+    ASSERT_TRUE(CLI::detail::checked_multiply(a, b));
+
+    a = 4;
+    b = std::numeric_limits<int>::min() / 4;
+    ASSERT_TRUE(CLI::detail::checked_multiply(a, b));
+
+    a = 48;
+    b = std::numeric_limits<int>::min() / 48;
+    ASSERT_TRUE(CLI::detail::checked_multiply(a, b));
 }
 TEST(CheckedMultiply, SizeT) {