diff --git a/src/cryfs/filesystem/CryNode.cpp b/src/cryfs/filesystem/CryNode.cpp index 4a0be4f1..9b8c70ca 100644 --- a/src/cryfs/filesystem/CryNode.cpp +++ b/src/cryfs/filesystem/CryNode.cpp @@ -57,22 +57,17 @@ void CryNode::rename(const bf::path &to) { //We are the root direcory. throw FuseErrnoException(EBUSY); } - //TODO More efficient implementation possible: directly rename when it's actually not moved to a different directory - // It's also quite ugly code because in the parent==targetDir case, it depends on _parent not overriding the changes made by targetDir. - auto optOld = (*_parent)->GetChild(_key); - if (optOld == boost::none) { - throw FuseErrnoException(ENOENT); - } - const auto &old = *optOld; - auto mode = old.mode(); - auto uid = old.uid(); - auto gid = old.gid(); - auto lastAccessTime = old.lastAccessTime(); - auto lastModificationTime = old.lastModificationTime(); - (*_parent)->RemoveChild(_key); - (*_parent)->flush(); auto targetDir = _device->LoadDirBlob(to.parent_path()); - targetDir->AddChild(to.filename().native(), _key, getType(), mode, uid, gid, lastAccessTime, lastModificationTime); + auto old = (*_parent)->GetChild(_key); + if (old == boost::none) { + throw FuseErrnoException(EIO); + } + std::string oldName = old->name(); // Store, because the next line invalidates the 'old' object + if (oldName != to.filename().native()) { + targetDir->AddChild(to.filename().native(), old->key(), old->type(), old->mode(), old->uid(), old->gid(), + old->lastAccessTime(), old->lastModificationTime()); + (*_parent)->RemoveChild(oldName); + } } void CryNode::utimens(timespec lastAccessTime, timespec lastModificationTime) { diff --git a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h index 7c5eed72..cee266b0 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h @@ -34,6 +34,10 @@ public: return _base->RemoveChild(key); } + void RemoveChild(const std::string &name) { + return _base->RemoveChild(name); + } + void flush() { return _base->flush(); } diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp index 849b3243..a7f266b1 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp @@ -93,6 +93,12 @@ boost::optional DirBlob::GetChild(const Key &key) const { return _entries.get(key); } +void DirBlob::RemoveChild(const string &name) { + std::unique_lock lock(_mutex); + _entries.remove(name); + _changed = true; +} + void DirBlob::RemoveChild(const Key &key) { std::unique_lock lock(_mutex); _entries.remove(key); diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.h b/src/cryfs/filesystem/fsblobstore/DirBlob.h index 316c00a9..592c276e 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.h +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.h @@ -46,6 +46,8 @@ namespace cryfs { void AddChild(const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType type, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); + void RemoveChild(const std::string &name); + void RemoveChild(const blockstore::Key &key); void flush(); diff --git a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp index bc4ec303..200b5722 100644 --- a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp +++ b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp @@ -59,9 +59,7 @@ void DirEntryList::add(const string &name, const Key &blobKey, fspp::Dir::EntryT } boost::optional DirEntryList::get(const string &name) const { - auto found = std::find_if(_entries.begin(), _entries.end(), [&name] (const DirEntry &entry) { - return entry.name() == name; - }); + auto found = _findByName(name); if (found == _entries.end()) { return boost::none; } @@ -69,19 +67,36 @@ boost::optional DirEntryList::get(const string &name) const { } boost::optional DirEntryList::get(const Key &key) const { - auto found = _find(key); + auto found = _findByKey(key); if (found == _entries.end()) { return boost::none; } return *found; } -void DirEntryList::remove(const Key &key) { - auto found = _find(key); +void DirEntryList::remove(const string &name) { + auto found = _findByName(name); + if (found == _entries.end()) { + throw fspp::fuse::FuseErrnoException(ENOENT); + } _entries.erase(found); } -vector::iterator DirEntryList::_find(const Key &key) { +void DirEntryList::remove(const Key &key) { + auto lowerBound = _findLowerBound(key); + auto upperBound = std::find_if(lowerBound, _entries.end(), [&key] (const DirEntry &entry) { + return entry.key() != key; + }); + _entries.erase(lowerBound, upperBound); +} + +vector::const_iterator DirEntryList::_findByName(const string &name) const { + return std::find_if(_entries.begin(), _entries.end(), [&name] (const DirEntry &entry) { + return entry.name() == name; + }); +} + +vector::iterator DirEntryList::_findByKey(const Key &key) { auto found = _findLowerBound(key); if (found == _entries.end() || found->key() != key) { throw fspp::fuse::FuseErrnoException(ENOENT); @@ -118,8 +133,8 @@ vector::iterator DirEntryList::_findFirst(const Key &hint, std::functi return iter; } -vector::const_iterator DirEntryList::_find(const Key &key) const { - return const_cast(this)->_find(key); +vector::const_iterator DirEntryList::_findByKey(const Key &key) const { + return const_cast(this)->_findByKey(key); } size_t DirEntryList::size() const { @@ -135,13 +150,13 @@ DirEntryList::const_iterator DirEntryList::end() const { } void DirEntryList::setMode(const Key &key, mode_t mode) { - auto found = _find(key); + auto found = _findByKey(key); ASSERT ((S_ISREG(mode) && S_ISREG(found->mode())) || (S_ISDIR(mode) && S_ISDIR(found->mode())) || (S_ISLNK(mode)), "Unknown mode in entry"); found->setMode(mode); } bool DirEntryList::setUidGid(const Key &key, uid_t uid, gid_t gid) { - auto found = _find(key); + auto found = _findByKey(key); bool changed = false; if (uid != (uid_t)-1) { found->setUid(uid); @@ -155,7 +170,7 @@ bool DirEntryList::setUidGid(const Key &key, uid_t uid, gid_t gid) { } void DirEntryList::setAccessTimes(const blockstore::Key &key, timespec lastAccessTime, timespec lastModificationTime) { - auto found = _find(key); + auto found = _findByKey(key); found->setLastAccessTime(lastAccessTime); found->setLastModificationTime(lastModificationTime); } diff --git a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h index 9d3bbed9..40a8541d 100644 --- a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h +++ b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h @@ -23,6 +23,7 @@ namespace cryfs { mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); boost::optional get(const std::string &name) const; boost::optional get(const blockstore::Key &key) const; + void remove(const std::string &name); void remove(const blockstore::Key &key); size_t size() const; @@ -36,8 +37,9 @@ namespace cryfs { private: uint64_t _serializedSize() const; bool _hasChild(const std::string &name) const; - std::vector::iterator _find(const blockstore::Key &key); - std::vector::const_iterator _find(const blockstore::Key &key) const; + std::vector::const_iterator _findByName(const std::string &name) const; + std::vector::iterator _findByKey(const blockstore::Key &key); + std::vector::const_iterator _findByKey(const blockstore::Key &key) const; std::vector::iterator _findUpperBound(const blockstore::Key &key); std::vector::iterator _findLowerBound(const blockstore::Key &key); std::vector::iterator _findFirst(const blockstore::Key &hint, std::function pred); diff --git a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h index 113b9be8..3fd9efe7 100644 --- a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h @@ -30,6 +30,10 @@ public: return _base->RemoveChild(key); } + void RemoveChild(const std::string &name) { + return _base->RemoveChild(name); + } + void flush() { return _base->flush(); } diff --git a/src/fspp/fs_interface/Node.h b/src/fspp/fs_interface/Node.h index 650f247a..9e7a297d 100644 --- a/src/fspp/fs_interface/Node.h +++ b/src/fspp/fs_interface/Node.h @@ -16,7 +16,7 @@ public: virtual void chmod(mode_t mode) = 0; virtual void chown(uid_t uid, gid_t gid) = 0; virtual void access(int mask) const = 0; - virtual void rename(const boost::filesystem::path &to) = 0; + virtual void rename(const boost::filesystem::path &to) = 0; // 'to' will always be an absolute path virtual void utimens(const timespec lastAccessTime, const timespec lastModificationTime) = 0; virtual void remove() = 0; }; diff --git a/src/fspp/fstest/FsTest.h b/src/fspp/fstest/FsTest.h index c403c0d3..16f2aa2b 100644 --- a/src/fspp/fstest/FsTest.h +++ b/src/fspp/fstest/FsTest.h @@ -7,13 +7,17 @@ #include "FsppDirTest.h" #include "FsppFileTest.h" #include "FsppSymlinkTest.h" +#include "FsppNodeTest.h" #include "FsppOpenFileTest.h" #define FSPP_ADD_FILESYTEM_TESTS(FS_NAME, FIXTURE) \ - INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppDeviceTest, FIXTURE); \ - INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppDirTest, FIXTURE); \ - INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppFileTest, FIXTURE); \ - INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppSymlinkTest, FIXTURE); \ - INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppOpenFileTest, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppDeviceTest, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppDirTest, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppFileTest, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppSymlinkTest, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppNodeTest_File, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppNodeTest_Dir, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppNodeTest_Symlink, FIXTURE); \ + INSTANTIATE_TYPED_TEST_CASE_P(FS_NAME, FsppOpenFileTest, FIXTURE); \ #endif diff --git a/src/fspp/fstest/FsppDirTest.h b/src/fspp/fstest/FsppDirTest.h index 13393da1..5c38c377 100644 --- a/src/fspp/fstest/FsppDirTest.h +++ b/src/fspp/fstest/FsppDirTest.h @@ -246,8 +246,6 @@ TYPED_TEST_P(FsppDirTest, CreateDir_AlreadyExisting) { ); } - - REGISTER_TYPED_TEST_CASE_P(FsppDirTest, Children_RootDir_Empty, Children_RootDir_OneFile_Directly, @@ -283,7 +281,7 @@ REGISTER_TYPED_TEST_CASE_P(FsppDirTest, //TODO chmod //TODO chown //TODO mkdir with uid/gid - -//TODO Test permission flags +//TODO createAndOpenFile: all stat values correctly set (1. in the OpenFile instance returned from createAndOpenFile and 2. on an lstat on the file object afterwards) +//TODO Test all operations do (or don't) affect dir timestamps correctly #endif diff --git a/src/fspp/fstest/FsppFileTest.h b/src/fspp/fstest/FsppFileTest.h index d87bd9ca..f7f6f870 100644 --- a/src/fspp/fstest/FsppFileTest.h +++ b/src/fspp/fstest/FsppFileTest.h @@ -271,7 +271,7 @@ REGISTER_TYPED_TEST_CASE_P(FsppFileTest, //TODO access //TODO rename //TODO unlink -//TODO createAndOpenFile (all stat values correctly set) -//TODO Test all operations do (or don't) affect file timestamps correctly +//TODO Test all operations do (or don't) affect file timestamps correctly (including rename, which shouldn't modify access/modify time, but inode change time) +//TODO (here and in other fstest operations): Test error paths #endif diff --git a/src/fspp/fstest/FsppNodeTest.h b/src/fspp/fstest/FsppNodeTest.h new file mode 100644 index 00000000..efe4ca08 --- /dev/null +++ b/src/fspp/fstest/FsppNodeTest.h @@ -0,0 +1,135 @@ +#pragma once +#ifndef MESSMER_FSPP_FSTEST_FSPPNODETEST_H_ +#define MESSMER_FSPP_FSTEST_FSPPNODETEST_H_ + +#include "testutils/FileSystemTest.h" +#include +#include +#include +#include "../fuse/FuseErrnoException.h" + +template +class FsppNodeTest: public FileSystemTest { +public: + virtual cpputils::unique_ref CreateNode(const boost::filesystem::path &path) = 0; + + void Test_Rename_TargetParentDirDoesntExist() { + auto node = CreateNode("/oldname"); + try { + node->rename("/notexistingdir/newname"); + EXPECT_TRUE(false); // Expect it throws an exception + } catch (const fspp::fuse::FuseErrnoException &e) { + EXPECT_EQ(ENOENT, e.getErrno()); + } + //Old file should still exist + EXPECT_NE(boost::none, this->device->Load("/oldname")); + } + + void Test_Rename_InRoot() { + auto node = CreateNode("/oldname"); + node->rename("/newname"); + EXPECT_EQ(boost::none, this->device->Load("/oldname")); + EXPECT_NE(boost::none, this->device->Load("/newname")); + } + + void Test_Rename_InNested() { + CreateDir("/mydir"); + auto node = CreateNode("/mydir/oldname"); + node->rename("/mydir/newname"); + EXPECT_EQ(boost::none, this->device->Load("/mydir/oldname")); + EXPECT_NE(boost::none, this->device->Load("/mydir/newname")); + } + + void Test_Rename_RootToNested() { + CreateDir("/mydir"); + auto node = CreateNode("/oldname"); + node->rename("/mydir/newname"); + EXPECT_EQ(boost::none, this->device->Load("/oldname")); + EXPECT_NE(boost::none, this->device->Load("/mydir/newname")); + } + + void Test_Rename_NestedToRoot() { + CreateDir("/mydir"); + auto node = CreateNode("/mydir/oldname"); + node->rename("/newname"); + EXPECT_EQ(boost::none, this->device->Load("/mydir/oldname")); + EXPECT_NE(boost::none, this->device->Load("/newname")); + } + + void Test_Rename_NestedToNested() { + CreateDir("/mydir"); + CreateDir("/mydir2"); + auto node = CreateNode("/mydir/oldname"); + node->rename("/mydir2/newname"); + EXPECT_EQ(boost::none, this->device->Load("/mydir/oldname")); + EXPECT_NE(boost::none, this->device->Load("/mydir2/newname")); + } + + void Test_Rename_ToItself() { + auto node = CreateNode("/oldname"); + node->rename("/oldname"); + EXPECT_NE(boost::none, this->device->Load("/oldname")); + } + +private: + void CreateDir(const boost::filesystem::path &path) { + this->LoadDir(path.parent_path())->createDir(path.filename().native(), this->MODE_PUBLIC, 0, 0); + } +}; + +template +class FsppNodeTest_File: public FsppNodeTest { +public: + cpputils::unique_ref CreateNode(const boost::filesystem::path &path) override { + this->LoadDir(path.parent_path())->createAndOpenFile(path.filename().native(), this->MODE_PUBLIC, 0, 0); + return this->LoadFile(path); + } +}; + +template +class FsppNodeTest_Dir: public FsppNodeTest { +public: + cpputils::unique_ref CreateNode(const boost::filesystem::path &path) override { + this->LoadDir(path.parent_path())->createDir(path.filename().native(), this->MODE_PUBLIC, 0, 0); + return this->LoadDir(path); + } +}; + +template +class FsppNodeTest_Symlink: public FsppNodeTest { +public: + cpputils::unique_ref CreateNode(const boost::filesystem::path &path) override { + this->LoadDir(path.parent_path())->createSymlink(path.filename().native(), "/my/symlink/target", 0, 0); + return this->LoadSymlink(path); + } +}; + +#define REGISTER_NODE_TEST_CASE(r, Class, Name) \ + TYPED_TEST_P(Class, Name) { \ + this->BOOST_PP_CAT(Test_,Name)(); \ + } \ + +#define REGISTER_NODE_TEST_CASES_FOR_CLASS(Class, ...) \ + TYPED_TEST_CASE_P(Class); \ + BOOST_PP_SEQ_FOR_EACH(REGISTER_NODE_TEST_CASE, Class, BOOST_PP_VARIADIC_TO_SEQ(__VA_ARGS__)); \ + REGISTER_TYPED_TEST_CASE_P(Class, __VA_ARGS__); \ + +#define REGISTER_NODE_TEST_CASES(...) \ + REGISTER_NODE_TEST_CASES_FOR_CLASS(FsppNodeTest_File, __VA_ARGS__); \ + REGISTER_NODE_TEST_CASES_FOR_CLASS(FsppNodeTest_Dir, __VA_ARGS__); \ + REGISTER_NODE_TEST_CASES_FOR_CLASS(FsppNodeTest_Symlink, __VA_ARGS__); \ + +/* + * Register each of the given test cases for all three classes: FsppNodeTest_File, FsppNodeTest_Dir and FsppNodeTest_Symlink + */ +REGISTER_NODE_TEST_CASES( + Rename_TargetParentDirDoesntExist, + Rename_InRoot, + Rename_InNested, + Rename_RootToNested, + Rename_NestedToRoot, + Rename_NestedToNested, + Rename_ToItself +); + +#endif diff --git a/src/fspp/fstest/FsppOpenFileTest.h b/src/fspp/fstest/FsppOpenFileTest.h index 7dc39f67..242d5fa0 100644 --- a/src/fspp/fstest/FsppOpenFileTest.h +++ b/src/fspp/fstest/FsppOpenFileTest.h @@ -26,5 +26,7 @@ REGISTER_TYPED_TEST_CASE_P(FsppOpenFileTest, //TODO Test flush //TODO Test fsync //TODO Test fdatasync +//TODO Test stat on file that was just created (i.e. the OpenFile instance returned by createAndOpenFile) +//TODO Test all operations do (or don't) affect file timestamps correctly #endif diff --git a/src/fspp/fstest/FsppSymlinkTest.h b/src/fspp/fstest/FsppSymlinkTest.h index 9dc624fa..30c49712 100644 --- a/src/fspp/fstest/FsppSymlinkTest.h +++ b/src/fspp/fstest/FsppSymlinkTest.h @@ -13,56 +13,33 @@ public: void CreateSymlink(const std::string &source, const boost::filesystem::path &target) { this->LoadDir("/")->createSymlink(source, target, 0, 0); } - - void Test_Create_AbsolutePath() { - CreateSymlink("mysymlink", "/my/symlink/target"); - } - - void Test_Create_RelativePath() { - CreateSymlink("mysymlink", "../target"); - } - - void Test_Read_AbsolutePath() { - CreateSymlink("mysymlink", "/my/symlink/target"); - EXPECT_EQ("/my/symlink/target", this->LoadSymlink("/mysymlink")->target()); - } - - void Test_Read_RelativePath() { - CreateSymlink("mysymlink", "../target"); - EXPECT_EQ("../target", this->LoadSymlink("/mysymlink")->target()); - } - - void Test_Delete() { - CreateSymlink("mysymlink", "/my/symlink/target"); - std::cerr << "1" << std::endl; - EXPECT_NE(boost::none, this->device->Load("/mysymlink")); - std::cerr << "2" << std::endl; - this->LoadSymlink("/mysymlink")->remove(); - std::cerr << "3" << std::endl; - EXPECT_EQ(boost::none, this->device->Load("/mysymlink")); - } }; TYPED_TEST_CASE_P(FsppSymlinkTest); TYPED_TEST_P(FsppSymlinkTest, Create_AbsolutePath) { - this->Test_Create_AbsolutePath(); + this->CreateSymlink("mysymlink", "/my/symlink/target"); } TYPED_TEST_P(FsppSymlinkTest, Create_RelativePath) { - this->Test_Create_RelativePath(); + this->CreateSymlink("mysymlink", "../target"); } TYPED_TEST_P(FsppSymlinkTest, Read_AbsolutePath) { - this->Test_Read_AbsolutePath(); + this->CreateSymlink("mysymlink", "/my/symlink/target"); + EXPECT_EQ("/my/symlink/target", this->LoadSymlink("/mysymlink")->target()); } TYPED_TEST_P(FsppSymlinkTest, Read_RelativePath) { - this->Test_Read_RelativePath(); + this->CreateSymlink("mysymlink", "../target"); + EXPECT_EQ("../target", this->LoadSymlink("/mysymlink")->target()); } TYPED_TEST_P(FsppSymlinkTest, Delete) { - this->Test_Delete(); + this->CreateSymlink("mysymlink", "/my/symlink/target"); + EXPECT_NE(boost::none, this->device->Load("/mysymlink")); + this->LoadSymlink("/mysymlink")->remove(); + EXPECT_EQ(boost::none, this->device->Load("/mysymlink")); } REGISTER_TYPED_TEST_CASE_P(FsppSymlinkTest, diff --git a/src/fspp/fstest/testutils/FileTest.h b/src/fspp/fstest/testutils/FileTest.h index b8d6c337..c15c00e1 100644 --- a/src/fspp/fstest/testutils/FileTest.h +++ b/src/fspp/fstest/testutils/FileTest.h @@ -16,6 +16,8 @@ public: this->LoadDir("/")->createDir("mydir", this->MODE_PUBLIC, 0, 0); this->LoadDir("/mydir")->createAndOpenFile("mynestedfile", this->MODE_PUBLIC, 0, 0); file_nested = cpputils::to_unique_ptr(this->LoadFile("/mydir/mynestedfile")); + + this->LoadDir("/")->createDir("mydir2", this->MODE_PUBLIC, 0, 0); } std::unique_ptr file_root; std::unique_ptr file_nested; diff --git a/src/fspp/fuse/Fuse.cpp b/src/fspp/fuse/Fuse.cpp index d22b53bb..ed571e69 100644 --- a/src/fspp/fuse/Fuse.cpp +++ b/src/fspp/fuse/Fuse.cpp @@ -418,6 +418,8 @@ int Fuse::rename(const bf::path &from, const bf::path &to) { LOG(DEBUG) << "rename(" << from << ", " << to << ")"; #endif try { + ASSERT(from.is_absolute(), "from has to be an absolute path"); + ASSERT(to.is_absolute(), "rename target has to be an absolute path. If this assert throws, we have to add code here that makes the path absolute."); _fs->rename(from, to); return 0; } catch(const cpputils::AssertFailed &e) {