common/strtol.cc: fix the coverity warnings

* promote the compared types properly to address the signed/unsigned
  comparison warnings. this also fixes the potential problems of
  slicing a compared type down to a "smaller" type before the
  comparison.
* check for the width of resulting type and shift bits caused by SI
  prefix to avoid the -Wshift-count-overflow warnings. this again
  is a potential issue, as shifting n bits of an integer of m bits
  width, where n >= m, leads to undefined behaviour.
* add a test for the 2nd fixed issue.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/common/strtol.cc b/src/common/strtol.cc
index fdab2cfe9afec7a0b8724481bf3517de48852b68..50598b9288e7ab64ebf267ee26c0df62b5351975 100644 (file)
--- a/src/common/strtol.cc
+++ b/src/common/strtol.cc
@@ -166,14 +166,21 @@ T strict_si_cast(const char *str, std::string *err)
     *err = "strict_sistrtoll: value should not be negative";
     return 0;
   }
-  if (ll < (long long)std::numeric_limits<T>::min() >> m) {
+  if (static_cast<unsigned>(m) >= sizeof(T) * CHAR_BIT) {
+    *err = ("strict_sistrtoll: the SI prefix is too large for the designated "
+           "type");
+    return 0;
+  }
+  using promoted_t = typename std::common_type<decltype(ll), T>::type;
+  if (static_cast<promoted_t>(ll) <
+      static_cast<promoted_t>(std::numeric_limits<T>::min()) >> m) {
     *err = "strict_sistrtoll: value seems to be too small";
     return 0;
   }
-  if (ll > std::numeric_limits<T>::max() >> m) {
+  if (static_cast<promoted_t>(ll) >
+      static_cast<promoted_t>(std::numeric_limits<T>::max()) >> m) {
     *err = "strict_sistrtoll: value seems to be too large";
     return 0;
-
   }
   return (ll << m);
 }

diff --git a/src/test/strtol.cc b/src/test/strtol.cc
index 93d6e68fb82958ee202fb8e975276a58131f9d04..646c055fb5d89daae2817779e7e35f95d890751d 100644 (file)
--- a/src/test/strtol.cc
+++ b/src/test/strtol.cc
@@ -215,6 +215,27 @@ TEST(SIStrToLL, Error) {
   test_strict_sistrtoll_err("1024E"); // overflows after adding the suffix
 }
 
+// since strict_sistrtoll is an alias of strict_si_cast<uint64_t>(), quite a few
+// of cases are covered by existing test cases of strict_sistrtoll already.
+TEST(StrictSICast, Error) {
+  {
+    std::string err;
+    // the SI prefix is way too large for `int`.
+    (void)strict_si_cast<int>("2E", &err);
+    ASSERT_NE(err, "");
+  }
+  {
+    std::string err;
+    (void)strict_si_cast<int>("-2E", &err);
+    ASSERT_NE(err, "");
+  }
+  {
+    std::string err;
+    (void)strict_si_cast<int>("1T", &err);
+    ASSERT_NE(err, "");
+  }
+}
+
 /*
  * Local Variables:
  * compile-command: "cd .. ; make unittest_strtol && ./unittest_strtol"