common: admin socket fallback to json-pretty format

If the format argument to a command sent to the admin socket is not
among the supported formats ( json, json-pretty, xml, xml-pretty ) the
new_formatter function will return null and the AdminSocketHook::call
function must fall back to a sensible default.

The CephContextHook::call and HelpHook::call failed to do that and a
malformed format argument would cause the mon to crash. A check is added
to each of them and fallback to json-pretty if the format is not
recognized.

To further protect AdminSocketHook::call implementations from similar
problems the format argument is checked immediately after accepting the
command in AdminSocket::do_accept and replaced with json-pretty if it is
not known.

A test case is added for both CephContextHook::call and HelpHook::call
to demonstrate the problem exists and is fixed by the patch.

Three other instances of unsafe calls to new_formatter were found and
a fallback to json-pretty was added. All other calls have been audited
and appear to be safe.

http://tracker.ceph.com/issues/7378 fixes #7378

Backport: emperor, dumpling
Signed-off-by: Loic Dachary <loic@dachary.org>

diff --git a/src/client/Client.cc b/src/client/Client.cc
index d7d9a507d3eac3be4351de8da8346172f77e52c2..ba39b1158aabf0e624e77d6f07697e8d574b71a7 100644 (file)
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -111,6 +111,8 @@ bool Client::CommandHook::call(std::string command, cmdmap_t& cmdmap,
                               std::string format, bufferlist& out)
 {
   Formatter *f = new_formatter(format);
+  if (!f)
+    f = new_formatter("json-pretty");
   f->open_object_section("result");
   m_client->client_lock.Lock();
   if (command == "mds_requests")

diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc
index 12e5868b409e3fe6cc1cbeb8240846829a9526b8..dc208c30f643d33b997335675a350909a4c62da5 100644 (file)
--- a/src/common/admin_socket.cc
+++ b/src/common/admin_socket.cc
@@ -321,6 +321,9 @@ bool AdminSocket::do_accept()
     return false;
   }
   cmd_getval(m_cct, cmdmap, "format", format);
+  if (format != "json" && format != "json-pretty" &&
+      format != "xml" && format != "xml-pretty")
+    format = "json-pretty";
   cmd_getval(m_cct, cmdmap, "prefix", c);
 
   string firstword;
@@ -444,6 +447,8 @@ public:
   HelpHook(AdminSocket *as) : m_as(as) {}
   bool call(string command, cmdmap_t &cmdmap, string format, bufferlist& out) {
     Formatter *f = new_formatter(format);
+    if (!f)
+      f = new_formatter("json-pretty");
     f->open_object_section("help");
     for (map<string,string>::iterator p = m_as->m_help.begin();
         p != m_as->m_help.end();

diff --git a/src/common/ceph_context.cc b/src/common/ceph_context.cc
index f7c2df5537e7c819eaa335a79cd4ba2e25ad87f9..4ebf79e82b9655052f4087058e8b1598dfc4ef4b 100644 (file)
--- a/src/common/ceph_context.cc
+++ b/src/common/ceph_context.cc
@@ -172,6 +172,8 @@ void CephContext::do_command(std::string command, cmdmap_t& cmdmap,
                             std::string format, bufferlist *out)
 {
   Formatter *f = new_formatter(format);
+  if (!f)
+    f = new_formatter("json-pretty");
   stringstream ss;
   for (cmdmap_t::iterator it = cmdmap.begin(); it != cmdmap.end(); ++it) {
     if (it->first != "prefix") {

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 38bb17194a36f5ebeb87be8884fb67ff6e917849..97f8f42aeda96b6e43f39ded1d60d29e44f49bc0 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -907,9 +907,9 @@ public:
 bool OSD::asok_command(string command, cmdmap_t& cmdmap, string format,
                       ostream& ss)
 {
-  if (format == "")
-    format = "json-pretty";
   Formatter *f = new_formatter(format);
+  if (!f)
+    f = new_formatter("json-pretty");
   if (command == "dump_ops_in_flight") {
     op_tracker.dump_ops_in_flight(f);
   } else if (command == "dump_historic_ops") {

diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc
index 3da9be9c1c032dac1fcd5822344dc994c64840c7..ce48fba050077899a37d1836837aca7165e72111 100644 (file)
--- a/src/osdc/Objecter.cc
+++ b/src/osdc/Objecter.cc
@@ -2507,6 +2507,8 @@ bool Objecter::RequestStateHook::call(std::string command, cmdmap_t& cmdmap,
                                      std::string format, bufferlist& out)
 {
   Formatter *f = new_formatter(format);
+  if (!f)
+    f = new_formatter("json-pretty");
   m_objecter->client_lock.Lock();
   m_objecter->dump_requests(f);
   m_objecter->client_lock.Unlock();

diff --git a/src/test/Makefile.am b/src/test/Makefile.am
index 83b83f4676ef1d30f47e7f51535a1a6b7213985c..84cc69e11c1c3b3dc940152a4ce3fba6bca38107 100644 (file)
--- a/src/test/Makefile.am
+++ b/src/test/Makefile.am
@@ -548,6 +548,11 @@ unittest_config_LDADD = $(UNITTEST_LDADD) $(CEPH_GLOBAL)
 unittest_config_CXXFLAGS = $(UNITTEST_CXXFLAGS)
 check_PROGRAMS += unittest_config
 
+unittest_context_SOURCES = test/common/test_context.cc
+unittest_context_LDADD = $(UNITTEST_LDADD) $(CEPH_GLOBAL)
+unittest_context_CXXFLAGS = $(UNITTEST_CXXFLAGS)
+check_PROGRAMS += unittest_context
+
 unittest_heartbeatmap_SOURCES = test/heartbeat_map.cc
 unittest_heartbeatmap_LDADD = $(LIBCOMMON) $(UNITTEST_LDADD) $(CEPH_GLOBAL)
 unittest_heartbeatmap_CXXFLAGS = $(UNITTEST_CXXFLAGS)

diff --git a/src/test/admin_socket.cc b/src/test/admin_socket.cc
index 6f2a215b565dfe9e176c7c85ef4eb5809bfd8e23..6484740674f6b555785ef2cbde5ccc76ee1e81ee 100644 (file)
--- a/src/test/admin_socket.cc
+++ b/src/test/admin_socket.cc
@@ -56,6 +56,38 @@ TEST(AdminSocket, TeardownSetup) {
   ASSERT_EQ(true, asoct.shutdown());
 }
 
+TEST(AdminSocket, SendHelp) {
+  std::auto_ptr<AdminSocket>
+      asokc(new AdminSocket(g_ceph_context));
+  AdminSocketTest asoct(asokc.get());
+  ASSERT_EQ(true, asoct.shutdown());
+  ASSERT_EQ(true, asoct.init(get_rand_socket_path()));
+  AdminSocketClient client(get_rand_socket_path());
+
+  {
+    string help;
+    ASSERT_EQ("", client.do_request("{\"prefix\":\"help\"}", &help));
+    ASSERT_NE(string::npos, help.find("\"list available commands\""));
+  }
+  {
+    string help;
+    ASSERT_EQ("", client.do_request("{"
+                                   " \"prefix\":\"help\","
+                                   " \"format\":\"xml\","
+                                   "}", &help));
+    ASSERT_NE(string::npos, help.find(">list available commands<"));
+  }
+  {
+    string help;
+    ASSERT_EQ("", client.do_request("{"
+                                   " \"prefix\":\"help\","
+                                   " \"format\":\"UNSUPPORTED\","
+                                   "}", &help));
+    ASSERT_NE(string::npos, help.find("\"list available commands\""));
+  }
+  ASSERT_EQ(true, asoct.shutdown());
+}
+
 TEST(AdminSocket, SendNoOp) {
   std::auto_ptr<AdminSocket>
       asokc(new AdminSocket(g_ceph_context));
@@ -146,3 +178,14 @@ TEST(AdminSocket, RegisterCommandPrefixes) {
   ASSERT_EQ("test| this thing", result);
   ASSERT_EQ(true, asoct.shutdown());
 }
+
+/*
+ * Local Variables:
+ * compile-command: "cd .. ;
+ *   make unittest_admin_socket &&
+ *    valgrind \
+ *    --max-stackframe=20000000 --tool=memcheck \
+ *   ./unittest_admin_socket # --gtest_filter=AdminSocket.*
+ * "
+ * End:
+ */

diff --git a/src/test/common/test_context.cc b/src/test/common/test_context.cc
new file mode 100644 (file)
index 0000000..ca745c9
--- /dev/null
+++ b/src/test/common/test_context.cc
@@ -0,0 +1,64 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+/*
+ * Ceph - scalable distributed file system
+ *
+ * Copyright (C) 2014 Cloudwatt <libre.licensing@cloudwatt.com>
+ *
+ * Author: Loic Dachary <loic@dachary.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Library Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Library Public License for more details.
+ *
+ *
+ */
+#include "gtest/gtest.h"
+#include "include/types.h"
+#include "include/msgr.h"
+#include "common/ceph_context.h"
+#include "common/config.h"
+
+TEST(CephContext, do_command)
+{
+  CephContext *cct = (new CephContext(CEPH_ENTITY_TYPE_CLIENT))->get();
+
+  string key("key");
+  string value("value");
+  cct->_conf->set_val(key.c_str(), value.c_str(), false);
+  cmdmap_t cmdmap;
+  cmdmap["var"] = key;
+
+  {
+    bufferlist out;
+    cct->do_command("config get", cmdmap, "xml", &out);
+    string s(out.c_str(), out.length());
+    EXPECT_EQ("<config get><key>" + value + "</key></config get>", s);
+  }
+
+  {
+    bufferlist out;
+    cct->do_command("config get", cmdmap, "UNSUPPORTED", &out);
+    string s(out.c_str(), out.length());
+    EXPECT_EQ("{ \"key\": \"value\"}", s);
+  }
+
+  cct->put();
+}
+
+/*
+ * Local Variables:
+ * compile-command: "cd ../.. ;
+ *   make unittest_context &&
+ *    valgrind \
+ *    --max-stackframe=20000000 --tool=memcheck \
+ *   ./unittest_context # --gtest_filter=CephContext.*
+ * "
+ * End:
+ */