crush: check for invalid names in loc[]

Add the is_valid_crush_loc helper to test for invalid crush names in
insert_item and update_item, before performing any side
effect. Implement the associated unit tests.

Signed-off-by: Loic Dachary <loic@dachary.org>

diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc
index 6ac90ccf6575689627ff6917b785a8874d95d9ce..a250d47fb64d76ce293a09941d4b54d081d0f959 100644 (file)
--- a/src/crush/CrushWrapper.cc
+++ b/src/crush/CrushWrapper.cc
@@ -336,16 +336,8 @@ int CrushWrapper::insert_item(CephContext *cct, int item, float weight, string n
   if (!is_valid_crush_name(name))
     return -EINVAL;
 
-
-  for (map<string,string>::const_iterator l = loc.begin(); l != loc.end(); l++) {
-    if (!is_valid_crush_name(l->second)) {
-      ldout(cct, 1) << "insert_item with loc["
-                    << l->first << "] = '"
-                    << l->second << "' is not a valid crush name ([A-Za-z0-9_-.]+)"
-                    << dendl;
-      return -ENFILE;
-    }
-  }
+  if (!is_valid_crush_loc(cct, loc))
+    return -EINVAL;
 
   if (name_exists(name)) {
     if (get_item_id(name) != item) {
@@ -517,6 +509,9 @@ int CrushWrapper::update_item(CephContext *cct, int item, float weight, string n
   if (!is_valid_crush_name(name))
     return -EINVAL;
 
+  if (!is_valid_crush_loc(cct, loc))
+    return -EINVAL;
+
   // compare quantized (fixed-point integer) weights!  
   int iweight = (int)(weight * (float)0x10000);
   int old_iweight;
@@ -1145,3 +1140,19 @@ bool CrushWrapper::is_valid_crush_name(const string& s)
   }
   return true;
 }
+
+bool CrushWrapper::is_valid_crush_loc(CephContext *cct,
+                                      const map<string,string> loc)
+{
+  for (map<string,string>::const_iterator l = loc.begin(); l != loc.end(); l++) {
+    if (!is_valid_crush_name(l->first) ||
+        !is_valid_crush_name(l->second)) {
+      ldout(cct, 1) << "loc["
+                    << l->first << "] = '"
+                    << l->second << "' not a valid crush name ([A-Za-z0-9_-.]+)"
+                    << dendl;
+      return false;
+    }
+  }
+  return true;
+}

diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h
index d665496b88f4ab878e4ce61d0cccc451a769a77c..9f5cbdfd3a63f63c92efa9fdca7772d1b29ae8ba 100644 (file)
--- a/src/crush/CrushWrapper.h
+++ b/src/crush/CrushWrapper.h
@@ -797,6 +797,8 @@ public:
 
 
   static bool is_valid_crush_name(const string& s);
+  static bool is_valid_crush_loc(CephContext *cct,
+                                const map<string,string> loc);
 };
 WRITE_CLASS_ENCODER(CrushWrapper)
 

diff --git a/src/test/crush/TestCrushWrapper.cc b/src/test/crush/TestCrushWrapper.cc
index 255ed648e98fcc37298a8dad16ab607326379469..1c6670eb3c9e5f43eaf035affbcf967a447b41d1 100644 (file)
--- a/src/test/crush/TestCrushWrapper.cc
+++ b/src/test/crush/TestCrushWrapper.cc
@@ -29,6 +29,35 @@
 
 #include "crush/CrushWrapper.h"
 
+TEST(CrushWrapper, update_item) {
+  CrushWrapper *c = new CrushWrapper;
+  c->create();
+
+#define ROOT_TYPE 3
+  c->set_type_name(ROOT_TYPE, "root");
+#define RACK_TYPE 2
+  c->set_type_name(RACK_TYPE, "rack");
+#define HOST_TYPE 1
+  c->set_type_name(HOST_TYPE, "host");
+#define OSD_TYPE 0
+  c->set_type_name(OSD_TYPE, "osd");
+
+  int rootno;
+  c->add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_RJENKINS1,
+               ROOT_TYPE, 0, NULL, NULL, &rootno);
+  c->set_item_name(rootno, "default");
+
+  int item = 0;
+
+  // invalid names anywhere in loc trigger an error
+  {
+    map<string,string> loc;
+    loc["rack"] = "\001";
+    EXPECT_EQ(-EINVAL, c->update_item(g_ceph_context, item, 1.0,
+                                     "osd." + stringify(item), loc));
+  }
+}
+
 TEST(CrushWrapper, insert_item) {
   CrushWrapper *c = new CrushWrapper;
   c->create();
@@ -48,6 +77,15 @@ TEST(CrushWrapper, insert_item) {
   c->set_item_name(rootno, "default");
 
   int item = 0;
+
+  // invalid names anywhere in loc trigger an error
+  {
+    map<string,string> loc;
+    loc["rack"] = "\001";
+    EXPECT_EQ(-EINVAL, c->insert_item(g_ceph_context, item, 1.0,
+                                     "osd." + stringify(item), loc));
+  }
+
   // insert an item in an existing bucket
   {
     map<string,string> loc;
@@ -71,16 +109,6 @@ TEST(CrushWrapper, insert_item) {
     EXPECT_EQ(0, c->insert_item(g_ceph_context, item, 1.0,
                                "osd." + stringify(item), loc));
   }
-  // implicit creation of a bucket with an invalid name fails
-  {
-    map<string,string> loc;
-    loc["root"] = "default";
-    loc["rack"] = "\001";
-
-    item++;
-    EXPECT_EQ(-ENFILE, c->insert_item(g_ceph_context, item, 1.0,
-                                     "osd." + stringify(item), loc));
-  }
   // adding to an existing item name that is not associated with a bucket
   {
     std::string name = "ITEM_WITHOUT_BUCKET";
@@ -200,6 +228,23 @@ TEST(CrushWrapper, is_valid_crush_name) {
   EXPECT_FALSE(CrushWrapper::is_valid_crush_name("\001"));
 }
 
+TEST(CrushWrapper, is_valid_crush_loc) {
+  map<string,string> loc;
+  EXPECT_TRUE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc));
+  loc["good"] = "better";
+  EXPECT_TRUE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc));
+  {
+    map<string,string> loc;
+    loc["\005"] = "default";
+    EXPECT_FALSE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc));
+  }
+  {
+    map<string,string> loc;
+    loc["rack"] = "\003";
+    EXPECT_FALSE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc));
+  }
+}
+
 int main(int argc, char **argv) {
   vector<const char*> args;
   argv_to_vec(argc, (const char **)argv, args);