mon: Properly handle errors from 'cmd_getval()' when needed

Not handling the error return from cmd_getval() may leave uninitialzied
values, which can cause issues, specially with non-string values.

Fixes: 6806
Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>

diff --git a/src/mon/MDSMonitor.cc b/src/mon/MDSMonitor.cc
index 1e81eba9ad802b4c7734e1a6b1f6964a612b3cd6..d25b0cb7778aa5a3b485579ac1d8b8f65e988efd 100644 (file)
--- a/src/mon/MDSMonitor.cc
+++ b/src/mon/MDSMonitor.cc
@@ -919,10 +919,19 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
 
   } else if (prefix == "mds set_state") {
     int64_t gid;
-    if (!cmd_getval(g_ceph_context, cmdmap, "gid", gid))
+    if (!cmd_getval(g_ceph_context, cmdmap, "gid", gid)) {
+      ss << "error parsing 'gid' value '"
+         << cmd_vartype_stringify(cmdmap["gid"]) << "'";
+      r = -EINVAL;
       goto out;
+    }
     int64_t state;
-    cmd_getval(g_ceph_context, cmdmap, "state", state);
+    if (!cmd_getval(g_ceph_context, cmdmap, "state", state)) {
+      ss << "error parsing 'state' string value '"
+         << cmd_vartype_stringify(cmdmap["state"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     if (!pending_mdsmap.is_dne_gid(gid)) {
       MDSMap::mds_info_t& info = pending_mdsmap.get_info_gid(gid);
       info.state = state;
@@ -946,7 +955,12 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
 
   } else if (prefix == "mds rm") {
     int64_t gid;
-    cmd_getval(g_ceph_context, cmdmap, "gid", gid);
+    if (!cmd_getval(g_ceph_context, cmdmap, "gid", gid)) {
+      ss << "error parsing 'gid' value '"
+         << cmd_vartype_stringify(cmdmap["gid"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     int state = pending_mdsmap.get_state_gid(gid);
     if (state == 0) {
       ss << "mds gid " << gid << " dne";
@@ -967,7 +981,12 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
     }
   } else if (prefix == "mds rmfailed") {
     int64_t w;
-    cmd_getval(g_ceph_context, cmdmap, "who", w);
+    if (!cmd_getval(g_ceph_context, cmdmap, "who", w)) {
+      ss << "error parsing 'who' value '"
+         << cmd_vartype_stringify(cmdmap["who"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     pending_mdsmap.failed.erase(w);
     stringstream ss;
     ss << "removed failed mds." << w;
@@ -994,7 +1013,12 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
     r = 0;
   } else if (prefix == "mds compat rm_compat") {
     int64_t f;
-    cmd_getval(g_ceph_context, cmdmap, "feature", f);
+    if (!cmd_getval(g_ceph_context, cmdmap, "feature", f)) {
+      ss << "error parsing feature value '"
+         << cmd_vartype_stringify(cmdmap["feature"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     if (pending_mdsmap.compat.compat.contains(f)) {
       ss << "removing compat feature " << f;
       pending_mdsmap.compat.compat.remove(f);
@@ -1005,7 +1029,12 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
     }
   } else if (prefix == "mds compat rm_incompat") {
     int64_t f;
-    cmd_getval(g_ceph_context, cmdmap, "feature", f);
+    if (!cmd_getval(g_ceph_context, cmdmap, "feature", f)) {
+      ss << "error parsing feature value '"
+         << cmd_vartype_stringify(cmdmap["feature"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     if (pending_mdsmap.compat.incompat.contains(f)) {
       ss << "removing incompat feature " << f;
       pending_mdsmap.compat.incompat.remove(f);
@@ -1064,8 +1093,18 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
   } else if (prefix == "mds newfs") {
     MDSMap newmap;
     int64_t metadata, data;
-    cmd_getval(g_ceph_context, cmdmap, "metadata", metadata);
-    cmd_getval(g_ceph_context, cmdmap, "data", data);
+    if (!cmd_getval(g_ceph_context, cmdmap, "metadata", metadata)) {
+      ss << "error parsing 'metadata' value '"
+         << cmd_vartype_stringify(cmdmap["metadata"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
+    if (!cmd_getval(g_ceph_context, cmdmap, "data", data)) {
+      ss << "error parsing 'data' value '"
+         << cmd_vartype_stringify(cmdmap["data"]) << "'";
+      r = -EINVAL;
+      goto out;
+    }
     string sure;
     cmd_getval(g_ceph_context, cmdmap, "sure", sure);
     if (sure != "--yes-i-really-mean-it") {

diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index a2d11120625b25ef35c520d1e827137e7d1b4b43..62533c53dd0cbdfb648f58ebadb1c63c639e4f92 100644 (file)
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -776,7 +776,11 @@ void Monitor::_osdmonitor_prepare_command(cmdmap_t& cmdmap, ostream& ss)
 void Monitor::_add_bootstrap_peer_hint(string cmd, cmdmap_t& cmdmap, ostream& ss)
 {
   string addrstr;
-  cmd_getval(g_ceph_context, cmdmap, "addr", addrstr);
+  if (!cmd_getval(g_ceph_context, cmdmap, "addr", addrstr)) {
+    ss << "unable to parse address string value '"
+         << cmd_vartype_stringify(cmdmap["addr"]) << "'";
+    return;
+  }
   dout(10) << "_add_bootstrap_peer_hint '" << cmd << "' '"
            << addrstr << "'" << dendl;
 

diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index b03144144a6fdd87b7c1ef8209e9ca9ccc862c9f..e757cfd798bc52ca7d2443ee4411017b3cef7488 100644 (file)
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -2209,7 +2209,12 @@ bool OSDMonitor::preprocess_command(MMonCommand *m)
     }
   } else if (prefix  == "osd find") {
     int64_t osd;
-    cmd_getval(g_ceph_context, cmdmap, "id", osd);
+    if (!cmd_getval(g_ceph_context, cmdmap, "id", osd)) {
+      ss << "unable to parse osd id value '"
+         << cmd_vartype_stringify(cmdmap["id"]) << "'";
+      r = -EINVAL;
+      goto reply;
+    }
     if (!osdmap.exists(osd)) {
       ss << "osd." << osd << " does not exist";
       r = -ENOENT;
@@ -2231,7 +2236,12 @@ bool OSDMonitor::preprocess_command(MMonCommand *m)
     f->flush(rdata);
   } else if (prefix == "osd metadata") {
     int64_t osd;
-    cmd_getval(g_ceph_context, cmdmap, "id", osd);
+    if (!cmd_getval(g_ceph_context, cmdmap, "id", osd)) {
+      ss << "unable to parse osd id value '"
+         << cmd_vartype_stringify(cmdmap["id"]) << "'";
+      r = -EINVAL;
+      goto reply;
+    }
     if (!osdmap.exists(osd)) {
       ss << "osd." << osd << " does not exist";
       r = -ENOENT;
@@ -3484,7 +3494,12 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
     }
 
     double weight;
-    cmd_getval(g_ceph_context, cmdmap, "weight", weight);
+    if (!cmd_getval(g_ceph_context, cmdmap, "weight", weight)) {
+      ss << "unable to parse weight value '"
+         << cmd_vartype_stringify(cmdmap["weight"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
 
     string args;
     vector<string> argvec;
@@ -3547,7 +3562,12 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
       }
 
       double weight;
-      cmd_getval(g_ceph_context, cmdmap, "weight", weight);
+      if (!cmd_getval(g_ceph_context, cmdmap, "weight", weight)) {
+        ss << "unable to parse weight value '"
+           << cmd_vartype_stringify(cmdmap["weight"]) << "'";
+        err = -EINVAL;
+        goto reply;
+      }
 
       string args;
       vector<string> argvec;
@@ -3741,7 +3761,12 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
        break;
       }
       double w;
-      cmd_getval(g_ceph_context, cmdmap, "weight", w);
+      if (!cmd_getval(g_ceph_context, cmdmap, "weight", w)) {
+        ss << "unable to parse weight value '"
+           << cmd_vartype_stringify(cmdmap["weight"]) << "'";
+        err = -EINVAL;
+        break;
+      }
 
       err = newcrush.adjust_item_weightf(g_ceph_context, id, w);
       if (err >= 0) {
@@ -3917,7 +3942,12 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
 
   } else if (prefix == "osd setmaxosd") {
     int64_t newmax;
-    cmd_getval(g_ceph_context, cmdmap, "newmax", newmax);
+    if (!cmd_getval(g_ceph_context, cmdmap, "newmax", newmax)) {
+      ss << "unable to parse 'newmax' value '"
+         << cmd_vartype_stringify(cmdmap["newmax"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
 
     if (newmax > g_conf->mon_max_osd) {
       err = -ERANGE;
@@ -4063,12 +4093,18 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
   } else if (prefix == "osd primary-affinity") {
     int64_t id;
     if (!cmd_getval(g_ceph_context, cmdmap, "id", id)) {
-      ss << "invalid osd id";
+      ss << "invalid osd id value '"
+         << cmd_vartype_stringify(cmdmap["id"]) << "'";
       err = -EINVAL;
       goto reply;
     }
     double w;
-    cmd_getval(g_ceph_context, cmdmap, "weight", w);
+    if (!cmd_getval(g_ceph_context, cmdmap, "weight", w)) {
+      ss << "unable to parse 'weight' value '"
+           << cmd_vartype_stringify(cmdmap["weight"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
     long ww = (int)((double)CEPH_OSD_MAX_PRIMARY_AFFINITY*w);
     if (ww < 0L) {
       ss << "weight must be >= 0";
@@ -4094,9 +4130,19 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
     }
   } else if (prefix == "osd reweight") {
     int64_t id;
-    cmd_getval(g_ceph_context, cmdmap, "id", id);
+    if (!cmd_getval(g_ceph_context, cmdmap, "id", id)) {
+      ss << "unable to parse osd id value '"
+         << cmd_vartype_stringify(cmdmap["id"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
     double w;
-    cmd_getval(g_ceph_context, cmdmap, "weight", w);
+    if (!cmd_getval(g_ceph_context, cmdmap, "weight", w)) {
+      ss << "unable to parse weight value '"
+         << cmd_vartype_stringify(cmdmap["weight"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
     long ww = (int)((double)CEPH_OSD_IN*w);
     if (ww < 0L) {
       ss << "weight must be >= 0";
@@ -4114,7 +4160,12 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
 
   } else if (prefix == "osd lost") {
     int64_t id;
-    cmd_getval(g_ceph_context, cmdmap, "id", id);
+    if (!cmd_getval(g_ceph_context, cmdmap, "id", id)) {
+      ss << "unable to parse osd id value '"
+         << cmd_vartype_stringify(cmdmap["id"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
     string sure;
     if (!cmd_getval(g_ceph_context, cmdmap, "sure", sure) || sure != "--yes-i-really-mean-it") {
       ss << "are you SURE?  this might mean real, permanent data loss.  pass "
@@ -4725,7 +4776,12 @@ done:
       goto reply;
     }
     int64_t size = 0;
-    cmd_getval(g_ceph_context, cmdmap, "size", size);
+    if (!cmd_getval(g_ceph_context, cmdmap, "size", size)) {
+      ss << "unable to parse 'size' value '"
+         << cmd_vartype_stringify(cmdmap["size"]) << "'";
+      err = -EINVAL;
+      goto reply;
+    }
     // make sure new tier is empty
     const pool_stat_t& tier_stats =
       mon->pgmon()->pg_map.get_pg_pool_sum_stat(tierpool_id);

diff --git a/src/mon/PGMonitor.cc b/src/mon/PGMonitor.cc
index 567be518938ad131d09ab672eaf87e59eff8a137..5adf5357d1e8b4c29ff385a75c81fde5234bd90f 100644 (file)
--- a/src/mon/PGMonitor.cc
+++ b/src/mon/PGMonitor.cc
@@ -1660,7 +1660,12 @@ bool PGMonitor::prepare_command(MMonCommand *m)
   } else if (prefix == "pg set_full_ratio" ||
             prefix == "pg set_nearfull_ratio") {
     double n;
-    cmd_getval(g_ceph_context, cmdmap, "ratio", n);
+    if (!cmd_getval(g_ceph_context, cmdmap, "ratio", n)) {
+      ss << "unable to parse 'ratio' value '"
+         << cmd_vartype_stringify(cmdmap["who"]) << "'";
+      r = -EINVAL;
+      goto reply;
+    }
     string op = prefix.substr(3, string::npos);
     if (op == "set_full_ratio")
       pending_inc.full_ratio = n;