From 77230d434a11dcbbe9d5540c45590c84ec7782be Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Mon, 25 Mar 2013 13:40:32 -0400 Subject: [PATCH] fix append to uninitialized buffer in FlatIndex::created The long_name variable is not initialized. When the append_oname function is called, it will strlen(long_name) and get a result that depends on the stack content. The long_name is truncated to a zero length string to prevent this unexpected behavior. There is no sure way to trigger the problem by writing a unit test. Unit tests are added for all public methods of the FlatIndex class. Most of the time the tests fail if the long_name variable is not properly initialized. * uint32_t collection_version() * coll_t coll() const * void set_ref(std::tr1::shared_ptr ref) * int cleanup() * int init() * int created(const hobject_t &hoid, const char *path) * int unlink(const hobject_t &hoid) * int lookup(const hobject_t &hoid, IndexedPath *path, int *exist) * int collection_list(vector *ls) * int collection_list_partial(const hobject_t &start, int min_count, int max_count, snapid_t seq, vector *ls, hobject_t *next) There are a number of border cases that cannot be tested, such as the logic of the lfn_get static function. Since FlatIndex code is designed to transition from older namespace conventions, it is difficult to figure out. The tests rely on xattr(2) and their availability is checked before running them. http://tracker.ceph.com/issues/4542 refs #4542 Signed-off-by: Loic Dachary --- src/Makefile.am | 6 ++ src/os/FlatIndex.cc | 1 + src/test/os/TestFlatIndex.cc | 141 +++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 src/test/os/TestFlatIndex.cc diff --git a/src/Makefile.am b/src/Makefile.am index fd54df867e214..4096f7d19c49e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -778,6 +778,12 @@ unittest_chain_xattr_LDADD = ${UNITTEST_STATIC_LDADD} $(LIBOS_LDA) $(LIBGLOBAL_ unittest_chain_xattr_CXXFLAGS = ${AM_CXXFLAGS} ${UNITTEST_CXXFLAGS} ${CRYPTO_CXXFLAGS} check_PROGRAMS += unittest_chain_xattr +unittest_flatindex_SOURCES = test/os/TestFlatIndex.cc +unittest_flatindex_LDFLAGS = ${AM_LDFLAGS} +unittest_flatindex_LDADD = ${UNITTEST_STATIC_LDADD} $(LIBOS_LDA) $(LIBGLOBAL_LDA) +unittest_flatindex_CXXFLAGS = ${AM_CXXFLAGS} ${UNITTEST_CXXFLAGS} ${CRYPTO_CXXFLAGS} +check_PROGRAMS += unittest_flatindex + unittest_strtol_SOURCES = test/strtol.cc unittest_strtol_LDFLAGS = $(PTHREAD_CFLAGS) ${AM_LDFLAGS} unittest_strtol_LDADD = ${UNITTEST_LDADD} $(LIBGLOBAL_LDA) diff --git a/src/os/FlatIndex.cc b/src/os/FlatIndex.cc index 9f40c367dcc17..f2e0713406042 100644 --- a/src/os/FlatIndex.cc +++ b/src/os/FlatIndex.cc @@ -280,6 +280,7 @@ int FlatIndex::init() { int FlatIndex::created(const hobject_t &hoid, const char *path) { char long_name[PATH_MAX]; + long_name[0] = '\0'; int actual_len = append_oname(hoid, long_name, sizeof(long_name)); if (actual_len < (int)FILENAME_PREFIX_LEN) { return 0; diff --git a/src/test/os/TestFlatIndex.cc b/src/test/os/TestFlatIndex.cc new file mode 100644 index 0000000000000..5d17c22a877b1 --- /dev/null +++ b/src/test/os/TestFlatIndex.cc @@ -0,0 +1,141 @@ +// -*- 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) 2013 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 +#include +#include "os/FlatIndex.h" +#include "os/CollectionIndex.h" +#include "os/chain_xattr.h" +#include "common/ceph_argparse.h" +#include "global/global_init.h" +#include + +TEST(FlatIndex, FlatIndex) { + coll_t collection("ABC"); + const std::string base_path("PATH"); + FlatIndex index(collection, base_path); + EXPECT_EQ(collection, index.coll()); + EXPECT_EQ((unsigned)0, index.collection_version()); + // + // checking placeholders + // + EXPECT_EQ(0, index.init()); + EXPECT_EQ(0, index.cleanup()); +} + +TEST(FlatIndex, collection) { + coll_t collection("ABC"); + const std::string base_path("PATH"); + FlatIndex index(collection, base_path); + const std::string key("KEY"); + uint64_t hash = 111; + uint64_t pool = 222; + const std::string object_name(10, 'A'); + hobject_t hoid(object_t(object_name), key, CEPH_NOSNAP, hash, pool); + vector ls; + ASSERT_DEATH(index.collection_list_partial(hoid, 0, 0, 0, &ls, &hoid), "0"); +} + +TEST(FlatIndex, created_unlink) { + coll_t collection("ABC"); + const std::string base_path("PATH"); + EXPECT_EQ(0, ::system("rm -fr PATH")); + EXPECT_EQ(0, ::mkdir("PATH", 0700)); + std::tr1::shared_ptr index(new FlatIndex(collection, base_path)); + const std::string key("KEY"); + uint64_t hash = 111; + uint64_t pool = 222; + // + // short object name + // + { + CollectionIndex::IndexedPath indexed_path; + index->set_ref(index); + const std::string object_name(10, 'A'); + hobject_t hoid(object_t(object_name), key, CEPH_NOSNAP, hash, pool); + int exists; + EXPECT_EQ(0, index->lookup(hoid, &indexed_path, &exists)); + EXPECT_EQ(0, exists); + EXPECT_EQ(0, ::close(::creat(indexed_path->path(), 0600))); + EXPECT_EQ(0, index->lookup(hoid, &indexed_path, &exists)); + EXPECT_EQ(1, exists); + EXPECT_EQ(0, index->unlink(hoid)); + EXPECT_EQ(0, index->lookup(hoid, &indexed_path, &exists)); + EXPECT_EQ(0, exists); + } + // + // long object name + // + { + CollectionIndex::IndexedPath indexed_path; + index->set_ref(index); + const std::string object_name(1024, 'A'); + hobject_t hoid(object_t(object_name), key, CEPH_NOSNAP, hash, pool); + int exists; + EXPECT_EQ(0, index->lookup(hoid, &indexed_path, &exists)); + EXPECT_EQ(0, exists); + EXPECT_EQ(0, ::close(::creat(indexed_path->path(), 0600))); + EXPECT_EQ(0, index->created(hoid, indexed_path->path())); + EXPECT_EQ(0, index->unlink(hoid)); + EXPECT_EQ(0, index->lookup(hoid, &indexed_path, &exists)); + EXPECT_EQ(0, exists); + } + EXPECT_EQ(0, ::system("rm -fr PATH")); +} + +TEST(FlatIndex, collection_list) { + coll_t collection("ABC"); + const std::string base_path("PATH"); + EXPECT_EQ(0, ::system("rm -fr PATH")); + EXPECT_EQ(0, ::mkdir("PATH", 0700)); + const std::string object_name("ABC"); + const std::string filename("PATH/" + object_name + "_head"); + EXPECT_EQ(0, ::close(::creat(filename.c_str(), 0600))); + std::tr1::shared_ptr index(new FlatIndex(collection, base_path)); + vector ls; + index->collection_list(&ls); + EXPECT_EQ((unsigned)1, ls.size()); + EXPECT_EQ(object_name, ls[0].oid.name); + EXPECT_EQ(0, ::system("rm -fr PATH")); +} + +int main(int argc, char **argv) { + int fd = ::creat("detect", 0600); + int ret = chain_fsetxattr(fd, "user.test", "A", 1); + ::close(fd); + ::unlink("detect"); + if (ret < 0) { + cerr << "SKIP FlatIndex because unable to test for xattr" << std::endl; + } else { + vector args; + argv_to_vec(argc, (const char **)argv, args); + + global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, CODE_ENVIRONMENT_UTILITY, 0); + common_init_finish(g_ceph_context); + + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); + } +} + +// Local Variables: +// compile-command: "cd ../.. ; make unittest_flatindex ; ./unittest_flatindex # --gtest_filter=FlatIndexTest.FlatIndex --log-to-stderr=true --debug-filestore=20" +// End: -- 2.39.5