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<CollectionIndex> 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<hobject_t> *ls)
 * int collection_list_partial(const hobject_t &start, int min_count, int max_count, snapid_t seq, vector<hobject_t> *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 <loic@dachary.org>

diff --git a/src/Makefile.am b/src/Makefile.am
index fd54df867e214e5a6e98dabb95ea2c425c59c344..4096f7d19c49e2e8662fcb2270e8ad6afd83a485 100644 (file)
--- 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 9f40c367dcc17fb494904447958c9b49b4d417a6..f2e0713406042c47d3695d5f84baefa4ed56e7da 100644 (file)
--- 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 (file)
index 0000000..5d17c22
--- /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 <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 <stdio.h>
+#include <signal.h>
+#include "os/FlatIndex.h"
+#include "os/CollectionIndex.h"
+#include "os/chain_xattr.h"
+#include "common/ceph_argparse.h"
+#include "global/global_init.h"
+#include <gtest/gtest.h>
+
+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<hobject_t> 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<CollectionIndex> 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<CollectionIndex> index(new FlatIndex(collection, base_path));
+  vector<hobject_t> 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<const char*> 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: