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/client/Client.cc b/src/client/Client.cc
index 76c1c91512655c99a9e7bf7f9cbbf23cc5584063..18a41fb0829b80b7eb3d2d863fca71b7a9e8651f 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,
 {
   stringstream ss;
   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 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 f8fb4c45c28d5e70841d6d7ab9a366db798c5633..3244dc88e2626dcd75135f9b598bdc62ca384f3c 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -976,9 +976,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 d2aa34897f9921b67edc4e52af3fd11a0ee3c6b9..c07cd659933303412693767078765f1bcf98c1ef 100644 (file)
--- a/src/osdc/Objecter.cc
+++ b/src/osdc/Objecter.cc
@@ -2410,6 +2410,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/Makefile.am b/src/test/Makefile.am
index 572d5d57dc441cac59d6beaeb60b1eaa8d5401ad..d753a37bc2f9bfbac0b678753905a9a44411ef06 100644 (file)
--- a/src/test/Makefile.am
+++ b/src/test/Makefile.am
@@ -500,6 +500,11 @@ unittest_confutils_LDADD = $(UNITTEST_LDADD) $(CEPH_GLOBAL)
 unittest_confutils_CXXFLAGS = $(UNITTEST_CXXFLAGS)
 check_PROGRAMS += unittest_confutils
 
+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:
+ */