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

Signed-off-by: Loic Dachary <loic@dachary.org>
(cherry picked from commit 165e76d4d03ffcc490fd3c2ba60fb37372990d0a)

diff --git a/src/Makefile.am b/src/Makefile.am
index 7909874170e13ea802dd0acfb0abc730656f355f..b72ea91b16e53e2f47e4d95a9898f6d25bb4c481 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -800,6 +800,12 @@ unittest_throttle_LDADD = libcommon.la ${LIBGLOBAL_LDA} ${UNITTEST_LDADD}
 unittest_throttle_CXXFLAGS = ${AM_CXXFLAGS} ${UNITTEST_CXXFLAGS} -O2
 check_PROGRAMS += unittest_throttle
 
+unittest_context_SOURCES = test/common/test_context.cc
+unittest_context_LDFLAGS = $(PTHREAD_CFLAGS) ${AM_LDFLAGS}
+unittest_context_LDADD = libcommon.la ${LIBGLOBAL_LDA} ${UNITTEST_LDADD}
+unittest_context_CXXFLAGS = ${AM_CXXFLAGS} ${UNITTEST_CXXFLAGS} -O2
+check_PROGRAMS += unittest_context
+
 unittest_base64_SOURCES = test/base64.cc
 unittest_base64_LDFLAGS = $(PTHREAD_CFLAGS) ${AM_LDFLAGS}
 unittest_base64_LDADD = libcephfs.la -lm ${UNITTEST_LDADD}

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 7cf3195365dd919c63cdf4d894ca2a70fe22a05b..016d3c079dfbeb9179ec4ba9472d077e122f4678 100644 (file)
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -107,6 +107,8 @@ bool Client::CommandHook::call(std::string command, cmdmap_t& cmdmap,
 {
   stringstream ss;
   Formatter *f = new_formatter(format);
+  if (!f)
+    f = new_formatter("json-pretty");
   m_client->client_lock.Lock();
   if (command == "mds_requests")
     m_client->dump_mds_requests(f);

diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc
index 1a507e606bfa0c8cd06a35b6e7bc2855de5c151b..23c6964ddd3cc1880c9770861d15399e76d84737 100644 (file)
--- a/src/common/admin_socket.cc
+++ b/src/common/admin_socket.cc
@@ -320,6 +320,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;
@@ -443,6 +446,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 e694a2f09b393a7b2583e85de9efa108032cbd5e..01f86dcce0dfa564843c1655927ad84a31d59fcc 100644 (file)
--- a/src/common/ceph_context.cc
+++ b/src/common/ceph_context.cc
@@ -168,6 +168,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 7bee1b6655da67450f67ac5e67cd6a6a8512e971..96b0b33906323ca96fbf691e5962e7cff51fb508 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -1018,9 +1018,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 b33ef277df440e9ffa4574386080b4407137bf35..0939196f727c3d84d9413115310ead1b23253eec 100644 (file)
--- a/src/osdc/Objecter.cc
+++ b/src/osdc/Objecter.cc
@@ -2357,6 +2357,8 @@ bool Objecter::RequestStateHook::call(std::string command, cmdmap_t& cmdmap,
 {
   stringstream ss;
   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/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:
+ */