From 30a604d2d6a5de7bcbfa0136e9a766cb34f96c0a Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Mon, 10 Feb 2014 23:42:38 +0100 Subject: [PATCH] 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 (cherry picked from commit 165e76d4d03ffcc490fd3c2ba60fb37372990d0a) --- src/Makefile.am | 6 ++++ src/client/Client.cc | 2 ++ src/common/admin_socket.cc | 5 +++ src/common/ceph_context.cc | 2 ++ src/osd/OSD.cc | 4 +-- src/osdc/Objecter.cc | 2 ++ src/test/admin_socket.cc | 43 ++++++++++++++++++++++ src/test/common/test_context.cc | 64 +++++++++++++++++++++++++++++++++ 8 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 src/test/common/test_context.cc diff --git a/src/Makefile.am b/src/Makefile.am index 7909874170e13..b72ea91b16e53 100644 --- 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 7cf3195365dd9..016d3c079dfbe 100644 --- 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 1a507e606bfa0..23c6964ddd3cc 100644 --- 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::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 e694a2f09b393..01f86dcce0dfa 100644 --- 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 7bee1b6655da6..96b0b33906323 100644 --- 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 b33ef277df440..0939196f727c3 100644 --- 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 6f2a215b565df..6484740674f6b 100644 --- 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 + 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 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 index 0000000000000..ca745c9a89bd1 --- /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 + * + * Author: Loic Dachary + * + * 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("" + value + "", 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: + */ -- 2.39.5