common/config: detect overflow of int values

* #include "strtol.h" in strtol.cc, to ensure the function defintions
  are consistent.
* add a test accordingly
* fix the testcase of converting 1024E.
* do not accept integers overflow after adding SI suffix
* do not accept integers underflow (i.e. negative values)

Fixes: #11484
Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit d62f80dc7b25d312ff05b65b7be854aae15b66a8)

diff --git a/src/common/config.cc b/src/common/config.cc
index 88f443d86e06cec1bd3081886946450774c0f6d6..f2460c636e27962d75f9fbc65329c3eb0e18bec8 100644 (file)
--- a/src/common/config.cc
+++ b/src/common/config.cc
@@ -877,7 +877,7 @@ int md_config_t::set_val_raw(const char *val, const config_option *opt)
   switch (opt->type) {
     case OPT_INT: {
       std::string err;
-      int f = strict_sistrtoll(val, &err);
+      int f = strict_si_cast<int>(val, &err);
       if (!err.empty())
        return -EINVAL;
       *(int*)opt->conf_ptr(this) = f;
@@ -885,7 +885,7 @@ int md_config_t::set_val_raw(const char *val, const config_option *opt)
     }
     case OPT_LONGLONG: {
       std::string err;
-      long long f = strict_sistrtoll(val, &err);
+      long long f = strict_si_cast<long long>(val, &err);
       if (!err.empty())
        return -EINVAL;
       *(long long*)opt->conf_ptr(this) = f;
@@ -915,7 +915,7 @@ int md_config_t::set_val_raw(const char *val, const config_option *opt)
       return 0;
     case OPT_U32: {
       std::string err;
-      int f = strict_sistrtoll(val, &err);
+      int f = strict_si_cast<int>(val, &err);
       if (!err.empty())
        return -EINVAL;
       *(uint32_t*)opt->conf_ptr(this) = f;
@@ -923,7 +923,7 @@ int md_config_t::set_val_raw(const char *val, const config_option *opt)
     }
     case OPT_U64: {
       std::string err;
-      long long f = strict_sistrtoll(val, &err);
+      uint64_t f = strict_si_cast<uint64_t>(val, &err);
       if (!err.empty())
        return -EINVAL;
       *(uint64_t*)opt->conf_ptr(this) = f;

diff --git a/src/common/strtol.cc b/src/common/strtol.cc
index 840b3d9c6a046df45c858818c8dde360e6953a75..8a43eb56be2a75c8a98b6d187eeb6c844f1bd27a 100644 (file)
--- a/src/common/strtol.cc
+++ b/src/common/strtol.cc
@@ -12,14 +12,12 @@
  *
  */
 
+#include "strtol.h"
+
 #include <errno.h>
 #include <limits.h>
 #include <sstream>
 #include <stdlib.h>
-#include <string>
-extern "C" {
-#include <stdint.h>
-}
 
 using std::ostringstream;
 
@@ -131,10 +129,8 @@ float strict_strtof(const char *str, std::string *err)
 uint64_t strict_sistrtoll(const char *str, std::string *err)
 {
   std::string s(str);
-  if (s.size() == 0) {
-    ostringstream oss;
-    oss << "strict_sistrtoll: value not specified";
-    *err = oss.str();
+  if (s.empty()) {
+    *err = "strict_sistrtoll: value not specified";
     return 0;
   }
   const char &u = s.at(s.size()-1); //str[std::strlen(str)-1];
@@ -161,9 +157,25 @@ uint64_t strict_sistrtoll(const char *str, std::string *err)
     s = std::string(str, s.size()-1);
   v = s.c_str();
 
-  uint64_t r = strict_strtoll(v, 10, err);
+  long long r_ll = strict_strtoll(v, 10, err);
+
+  if (r_ll < 0) {
+    *err = "strict_sistrtoll: value should not be negative";
+    return 0;
+  }
+
+  uint64_t r = r_ll;
   if (err->empty() && m > 0) {
-    r = (r << m);
+    if (r > (std::numeric_limits<uint64_t>::max() >> m)) {
+      *err = "strict_sistrtoll: value seems to be too large";
+      return 0;
+    }
+    r <<= m;
   }
   return r;
 }
+
+template <>
+uint64_t strict_si_cast(const char *str, std::string *err) {
+  return strict_sistrtoll(str, err);
+}

diff --git a/src/common/strtol.h b/src/common/strtol.h
index ea0a469a777e61e59aedb044861b66be10c87af2..5575ed7b390be92a3885e17c9069f4bf5ddb7a45 100644 (file)
--- a/src/common/strtol.h
+++ b/src/common/strtol.h
@@ -16,6 +16,7 @@
 #define CEPH_COMMON_STRTOL_H
 
 #include <string>
+#include <limits>
 extern "C" {
 #include <stdint.h>
 }
@@ -30,4 +31,21 @@ float strict_strtof(const char *str, std::string *err);
 
 uint64_t strict_sistrtoll(const char *str, std::string *err);
 
+template <typename Target>
+Target strict_si_cast(const char *str, std::string *err) {
+  uint64_t ret = strict_sistrtoll(str, err);
+  if (!err->empty())
+    return ret;
+  if (ret > (uint64_t)std::numeric_limits<Target>::max()) {
+    err->append("The option value '");
+    err->append(str);
+    err->append("' seems to be too large");
+    return 0;
+  }
+  return ret;
+}
+
+template <>
+uint64_t strict_si_cast(const char *str, std::string *err);
+
 #endif

diff --git a/src/test/daemon_config.cc b/src/test/daemon_config.cc
index 1361ddc22477c32d6b69032447ccac64e2f8b4e2..2968b3514a2faf756a9d54db75d8e2b595d59bc4 100644 (file)
--- a/src/test/daemon_config.cc
+++ b/src/test/daemon_config.cc
@@ -23,6 +23,9 @@
 #include <string>
 #include <string.h>
 
+#include <boost/lexical_cast.hpp>
+
+
 using std::string;
 
 TEST(DaemonConfig, SimpleSet) {
@@ -333,6 +336,30 @@ TEST(DaemonConfig, ThreadSafety1) {
                                       "false"));
   ASSERT_EQ(ret, 0);
 }
+
+TEST(DaemonConfig, InvalidIntegers) {
+  {
+    int ret = g_ceph_context->_conf->set_val("num_client", "-1");
+    ASSERT_EQ(ret, -EINVAL);
+  }
+  {
+    int ret = g_ceph_context->_conf->set_val("num_client", "-1K");
+    ASSERT_EQ(ret, -EINVAL);
+  }
+  {
+    long long bad_value = (long long)std::numeric_limits<int>::max() + 1;
+    string str = boost::lexical_cast<string>(bad_value);
+    int ret = g_ceph_context->_conf->set_val("num_client", str);
+    ASSERT_EQ(ret, -EINVAL);
+  }
+  {
+    // 4G must be greater than INT_MAX
+    ASSERT_GT(4LL * 1024 * 1024 * 1024, (long long)std::numeric_limits<int>::max());
+    int ret = g_ceph_context->_conf->set_val("num_client", "4G");
+    ASSERT_EQ(ret, -EINVAL);
+  }
+}
+
 /*
  * Local Variables:
  * compile-command: "cd .. ; make unittest_daemon_config && ./unittest_daemon_config"

diff --git a/src/test/strtol.cc b/src/test/strtol.cc
index 08ba081ac2bbf8a3702ea104a57c2a0d1bd91604..93d6e68fb82958ee202fb8e975276a58131f9d04 100644 (file)
--- a/src/test/strtol.cc
+++ b/src/test/strtol.cc
@@ -174,7 +174,8 @@ TEST(SIStrToLL, WithUnits) {
 
   for (std::map<char,int>::iterator p = units.begin();
        p != units.end(); ++p) {
-    test_strict_sistrtoll_units("1024", p->first, p->second);
+    // the upper bound of uint64_t is 2^64 = 4E
+    test_strict_sistrtoll_units("4", p->first, p->second);
     test_strict_sistrtoll_units("1", p->first, p->second);
     test_strict_sistrtoll_units("0", p->first, p->second);
   }
@@ -208,4 +209,14 @@ TEST(SIStrToLL, Error) {
   test_strict_sistrtoll_err("BM");
   test_strict_sistrtoll_err("B0wef");
   test_strict_sistrtoll_err("0m");
+  test_strict_sistrtoll_err("-1"); // it returns uint64_t
+  test_strict_sistrtoll_err("-1K");
+  // the upper bound of uint64_t is 2^64 = 4E, so 1024E overflows
+  test_strict_sistrtoll_err("1024E"); // overflows after adding the suffix
 }
+
+/*
+ * Local Variables:
+ * compile-command: "cd .. ; make unittest_strtol && ./unittest_strtol"
+ * End:
+ */