From c344ff170d278be6fc279c1275e8f17b14c72183 Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Sat, 30 Mar 2013 11:26:12 +0100 Subject: [PATCH] fix null character in object name triggering segfault Parsing \n in lfn_parse_object_name is implemented with out->append('\0'); which segfaults when using libstdc++ and g++ version 4.6.3 on Debian GNU/Linux. It is replaced with (*out) += '\0'; to avoid the bugous implicit conversion. There is no append(charT) method in C++98 or C++11, which means it relies on an implicit conversion that is bugous. It would be better to rely on the basic_string& operator+=(charT c); method as defined in ISO 14882-1998 (page 385) thru ISO 14882-2012 (page 640) A set of tests is added to generate and parse object names. They need access to the private function lfn_parse_object_name because there is no convenient protected method to exercise it. The tests contain a LFNIndex derived class, TestWrapLFNIndex which is made a friend of LFNIndex to gain access to the private methods. http://tracker.ceph.com/issues/4594 refs #4594 Signed-off-by: Loic Dachary --- src/os/LFNIndex.cc | 2 +- src/os/LFNIndex.h | 2 + src/test/os/TestLFNIndex.cc | 87 ++++++++++++++++++++++++------------- 3 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/os/LFNIndex.cc b/src/os/LFNIndex.cc index f1ba8c158f0b0..62b8f7085e16b 100644 --- a/src/os/LFNIndex.cc +++ b/src/os/LFNIndex.cc @@ -912,7 +912,7 @@ static bool append_unescaped(string::const_iterator begin, else if (*i == 's') out->append("/"); else if (*i == 'n') - out->append('\0'); + (*out) += '\0'; else if (*i == 'u') out->append("_"); else diff --git a/src/os/LFNIndex.h b/src/os/LFNIndex.h index b3c053588222c..b73ff4db268ff 100644 --- a/src/os/LFNIndex.h +++ b/src/os/LFNIndex.h @@ -577,6 +577,8 @@ private: char *hash, ///< [out] Hash of filename. int len ///< [in] Size of hash buffer. ); ///< @return Error Code, 0 on success. + + friend class TestWrapLFNIndex; }; typedef LFNIndex::IndexedPath IndexedPath; diff --git a/src/test/os/TestLFNIndex.cc b/src/test/os/TestLFNIndex.cc index 864c88c03b524..3666de0ede153 100644 --- a/src/test/os/TestLFNIndex.cc +++ b/src/test/os/TestLFNIndex.cc @@ -27,11 +27,11 @@ #include "global/global_init.h" #include -class TestIndex : public LFNIndex { +class TestWrapLFNIndex : public LFNIndex { public: - TestIndex(coll_t collection, - const char *base_path, - uint32_t index_version) : LFNIndex(collection, base_path, index_version) {} + TestWrapLFNIndex(coll_t collection, + const char *base_path, + uint32_t index_version) : LFNIndex(collection, base_path, index_version) {} virtual uint32_t collection_version() { return index_version; @@ -45,6 +45,14 @@ public: std::tr1::shared_ptr dest ) { return 0; } + void test_generate_and_parse(const hobject_t &hoid, const std::string &mangled_expected) { + const std::string mangled_name = lfn_generate_object_name(hoid); + EXPECT_EQ(mangled_expected, mangled_name); + hobject_t hoid_parsed; + EXPECT_TRUE(lfn_parse_object_name(mangled_name, &hoid_parsed)); + EXPECT_EQ(hoid, hoid_parsed); + } + protected: virtual int _init() { return 0; } @@ -81,9 +89,51 @@ protected: ) { return 0; } }; -class TestLFNIndex : public TestIndex, public ::testing::Test { +class TestHASH_INDEX_TAG : public TestWrapLFNIndex, public ::testing::Test { +public: + TestHASH_INDEX_TAG() : TestWrapLFNIndex(coll_t("ABC"), "PATH", CollectionIndex::HASH_INDEX_TAG) { + } +}; + +TEST_F(TestHASH_INDEX_TAG, generate_and_parse_name) { + const vector path; + const std::string key; + uint64_t hash = 0xABABABAB; + uint64_t pool = -1; + + test_generate_and_parse(hobject_t(object_t(".A/B_\\C.D"), key, CEPH_NOSNAP, hash, pool), + "\\.A\\sB_\\\\C.D_head_ABABABAB"); + test_generate_and_parse(hobject_t(object_t("DIR_A"), key, CEPH_NOSNAP, hash, pool), + "\\dA_head_ABABABAB"); +} + +class TestHASH_INDEX_TAG_2 : public TestWrapLFNIndex, public ::testing::Test { +public: + TestHASH_INDEX_TAG_2() : TestWrapLFNIndex(coll_t("ABC"), "PATH", CollectionIndex::HASH_INDEX_TAG_2) { + } +}; + +TEST_F(TestHASH_INDEX_TAG_2, generate_and_parse_name) { + const vector path; + std::string mangled_name; + const std::string key("KEY"); + uint64_t hash = 0xABABABAB; + uint64_t pool = -1; + + { + std::string name(".XA/B_\\C.D"); + name[1] = '\0'; + hobject_t hoid(object_t(name), key, CEPH_NOSNAP, hash, pool); + + test_generate_and_parse(hoid, "\\.\\nA\\sB\\u\\\\C.D_KEY_head_ABABABAB"); + } + test_generate_and_parse(hobject_t(object_t("DIR_A"), key, CEPH_NOSNAP, hash, pool), + "\\dA_KEY_head_ABABABAB"); +} + +class TestLFNIndex : public TestWrapLFNIndex, public ::testing::Test { public: - TestLFNIndex() : TestIndex(coll_t("ABC"), "PATH", 1) { + TestLFNIndex() : TestWrapLFNIndex(coll_t("ABC"), "PATH", CollectionIndex::HASH_INDEX_TAG) { } virtual void SetUp() { @@ -233,29 +283,6 @@ TEST_F(TestLFNIndex, remove_object) { TEST_F(TestLFNIndex, get_mangled_name) { const vector path; - // - // object name escape logic - // - { - std::string mangled_name; - int exists = 666; - - hobject_t hoid(sobject_t(".A/B_\\C.D", CEPH_NOSNAP)); - - EXPECT_EQ(0, get_mangled_name(path, hoid, &mangled_name, &exists)); - EXPECT_NE(std::string::npos, mangled_name.find("\\.A\\sB_\\\\C.D_head")); - EXPECT_EQ(0, exists); - } - { - std::string mangled_name; - int exists = 666; - - hobject_t hoid(sobject_t("DIR_A", CEPH_NOSNAP)); - - EXPECT_EQ(0, get_mangled_name(path, hoid, &mangled_name, &exists)); - EXPECT_NE(std::string::npos, mangled_name.find("\\dA_head")); - EXPECT_EQ(0, exists); - } // // small object name // @@ -376,5 +403,5 @@ int main(int argc, char **argv) { } // Local Variables: -// compile-command: "cd ../.. ; make unittest_lfnindex ; ./unittest_lfnindex # --gtest_filter=TestLFNIndex.* --log-to-stderr=true --debug-filestore=20" +// compile-command: "cd ../.. ; make unittest_lfnindex ; valgrind --tool=memcheck ./unittest_lfnindex # --gtest_filter=TestLFNIndex.* --log-to-stderr=true --debug-filestore=20" // End: -- 2.39.5