From 2b543918994ceb2271b4589ba5022b3da922ed67 Mon Sep 17 00:00:00 2001 From: Sam Lang Date: Tue, 25 Sep 2012 14:48:32 -0700 Subject: [PATCH 1/4] client: Fix to client filepath initializing The filepath constructor that takes a const char * is missing the initializer for the encoded member. This results in uninitialized memory, so the encoded field is sometimes true, resulting in mds crashes (see #3186) and client errors with empty components in path names. This commit fixes #3186 and #2285. Signed-off-by: Sam Lang --- src/include/filepath.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/filepath.h b/src/include/filepath.h index 88470759197..ec1519d5464 100644 --- a/src/include/filepath.h +++ b/src/include/filepath.h @@ -90,7 +90,7 @@ class filepath { * if we are fed a relative path as a string, either set ino=0 (strictly * relative) or 1 (absolute). throw out any leading '/'. */ - filepath(const char *s) { + filepath(const char *s) : encoded(false) { set_path(s); } void set_path(const char *s) { From c92e1dd7b0106fa986985d8179a8ed66c61e6709 Mon Sep 17 00:00:00 2001 From: Sam Lang Date: Tue, 25 Sep 2012 17:55:05 -0700 Subject: [PATCH 2/4] mds: Handle empty relpath from client getattr A bug in the client (see 2b54391) results in an empty relpath on a lookup request. This causes a segfault in the mds, because the getattr logic expects a lookup to have a relpath to place in the response. The fix here ensures that lookups include a non-empty relpath, o.w. aborting the request and returning -EINVAL to the client. Signed-off-by: Sam Lang --- src/mds/Server.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/mds/Server.cc b/src/mds/Server.cc index dcdc75add0d..7b928948609 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -1872,6 +1872,12 @@ CInode* Server::rdlock_path_pin_ref(MDRequest *mdr, int n, if (mdr->done_locking) return mdr->in[n]; + if (!no_lookup && 0 == refpath.depth()) { + // refpath can't be empty for lookup but it can for + // getattr (we do getattr with empty refpath for mount of '/') + reply_request(mdr, -EINVAL); + return 0; + } // traverse int r = mdcache->path_traverse(mdr, NULL, NULL, refpath, &mdr->dn[n], &mdr->in[n], MDS_TRAVERSE_FORWARD); From 26882dace73c90c8b844e5fb98a29598dfd3bb17 Mon Sep 17 00:00:00 2001 From: Sam Lang Date: Tue, 25 Sep 2012 18:14:12 -0700 Subject: [PATCH 3/4] client: Put all libcephfs gtest tests into one bin * Modify the Makefile.am to run all gtest libcephfs tests through a single binary: test_libcephfs. * Add tests for #2285, #3186, and #2778 Signed-off-by: Sam Lang --- src/Makefile.am | 10 +++--- src/test/libcephfs/test.cc | 67 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 src/test/libcephfs/test.cc diff --git a/src/Makefile.am b/src/Makefile.am index cd8e23322ec..9d338eb59e0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -854,11 +854,11 @@ test_rados_api_misc_LDADD = librados.la ${UNITTEST_STATIC_LDADD} test_rados_api_misc_CXXFLAGS = ${AM_CXXFLAGS} ${UNITTEST_CXXFLAGS} bin_DEBUGPROGRAMS += test_rados_api_misc -test_libcephfs_readdir_SOURCES = test/libcephfs/readdir_r_cb.cc -test_libcephfs_readdir_LDFLAGS = $(PTHREAD_CFLAGS) ${AM_LDFLAGS} -test_libcephfs_readdir_LDADD = ${UNITTEST_STATIC_LDADD} libcephfs.la -test_libcephfs_readdir_CXXFLAGS = $(AM_CXXFLAGS) ${UNITTEST_CXXFLAGS} -bin_DEBUGPROGRAMS += test_libcephfs_readdir +test_libcephfs_SOURCES = test/libcephfs/test.cc test/libcephfs/readdir_r_cb.cc +test_libcephfs_LDFLAGS = $(PTHREAD_CFLAGS) ${AM_LDFLAGS} +test_libcephfs_LDADD = ${UNITTEST_STATIC_LDADD} libcephfs.la +test_libcephfs_CXXFLAGS = $(AM_CXXFLAGS) ${UNITTEST_CXXFLAGS} +bin_DEBUGPROGRAMS += test_libcephfs test_filestore_SOURCES = test/filestore/store_test.cc test_filestore_LDFLAGS = ${AM_LDFLAGS} diff --git a/src/test/libcephfs/test.cc b/src/test/libcephfs/test.cc new file mode 100644 index 00000000000..af79b72c5ab --- /dev/null +++ b/src/test/libcephfs/test.cc @@ -0,0 +1,67 @@ +// -*- 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) 2011 New Dream Network + * + * This is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2.1, as published by the Free Software + * Foundation. See file COPYING. + * + */ + +#include "gtest/gtest.h" +#include "include/cephfs/libcephfs.h" +#include +#include +#include +#include + +TEST(LibCephFS, Open_empty_component) { + + pid_t mypid = getpid(); + struct ceph_mount_info *cmount; + ASSERT_EQ(0, ceph_create(&cmount, NULL)); + ASSERT_EQ(0, ceph_conf_read_file(cmount, NULL)); + ASSERT_EQ(0, ceph_mount(cmount, "/")); + + char c_dir[1024]; + sprintf(c_dir, "/open_test_%d", mypid); + struct ceph_dir_result *dirp; + + ASSERT_EQ(0, ceph_mkdirs(cmount, c_dir, 0777)); + + ASSERT_EQ(0, ceph_opendir(cmount, c_dir, &dirp)); + + char c_path[1024]; + sprintf(c_path, "/open_test_%d//created_file_%d", mypid, mypid); + int fd = ceph_open(cmount, c_path, O_RDONLY|O_CREAT, 0666); + ASSERT_LT(0, fd); + + ASSERT_EQ(0, ceph_close(cmount, fd)); + ASSERT_EQ(0, ceph_closedir(cmount, dirp)); + ceph_shutdown(cmount); + + ASSERT_EQ(0, ceph_create(&cmount, NULL)); + ASSERT_EQ(0, ceph_conf_read_file(cmount, NULL)); + + ASSERT_EQ(0, ceph_mount(cmount, "/")); + + fd = ceph_open(cmount, c_path, O_RDONLY, 0666); + ASSERT_LT(0, fd); + + ASSERT_EQ(0, ceph_close(cmount, fd)); + ASSERT_EQ(0, ceph_closedir(cmount, dirp)); + ceph_shutdown(cmount); +} + +TEST(LibCephFS, Mount_non_exist) { + + struct ceph_mount_info *cmount; + + ASSERT_EQ(0, ceph_create(&cmount, NULL)); + ASSERT_EQ(0, ceph_conf_read_file(cmount, NULL)); + ASSERT_NE(0, ceph_mount(cmount, "/non-exist")); +} From a9e304115fabf3dcbe0e7b9d8bad598b77530bee Mon Sep 17 00:00:00 2001 From: Sam Lang Date: Wed, 26 Sep 2012 16:04:50 -0700 Subject: [PATCH 4/4] mds: Move check for empty path lookup to getattr Signed-off-by: Sam Lang --- src/mds/Server.cc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 7b928948609..c175722a4eb 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -1872,13 +1872,6 @@ CInode* Server::rdlock_path_pin_ref(MDRequest *mdr, int n, if (mdr->done_locking) return mdr->in[n]; - if (!no_lookup && 0 == refpath.depth()) { - // refpath can't be empty for lookup but it can for - // getattr (we do getattr with empty refpath for mount of '/') - reply_request(mdr, -EINVAL); - return 0; - } - // traverse int r = mdcache->path_traverse(mdr, NULL, NULL, refpath, &mdr->dn[n], &mdr->in[n], MDS_TRAVERSE_FORWARD); if (r > 0) @@ -2101,6 +2094,14 @@ void Server::handle_client_getattr(MDRequest *mdr, bool is_lookup) { MClientRequest *req = mdr->client_request; set rdlocks, wrlocks, xlocks; + + if (req->get_filepath().depth() == 0 && is_lookup) { + // refpath can't be empty for lookup but it can for + // getattr (we do getattr with empty refpath for mount of '/') + reply_request(mdr, -EINVAL); + return; + } + CInode *ref = rdlock_path_pin_ref(mdr, 0, rdlocks, false, false, NULL, !is_lookup); if (!ref) return;