From 811c163bfa39d9135d0a75af2853c121ca552bfa Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 25 Apr 2016 17:42:17 -0700 Subject: [PATCH] Fix rename() when overwriting an existing file: (a) Keep the invariant that the list of directory entries is sorted and (b) delete the blob of the overwritten file --- src/cryfs/filesystem/CryDevice.cpp | 4 ++ src/cryfs/filesystem/CryDevice.h | 2 + src/cryfs/filesystem/CryNode.cpp | 5 ++- src/cryfs/filesystem/CryNode.h | 3 +- .../cachingfsblobstore/DirBlobRef.h | 5 ++- src/cryfs/filesystem/fsblobstore/DirBlob.cpp | 12 ++++-- src/cryfs/filesystem/fsblobstore/DirBlob.h | 3 +- .../fsblobstore/utils/DirEntryList.cpp | 13 ++++-- .../fsblobstore/utils/DirEntryList.h | 5 ++- .../parallelaccessfsblobstore/DirBlobRef.h | 5 ++- test/cryfs/CMakeLists.txt | 1 + test/cryfs/filesystem/CryNodeTest.cpp | 43 +++++++++++++++++++ test/cryfs/filesystem/testutils/CryTestBase.h | 33 ++++++++++++++ 13 files changed, 117 insertions(+), 17 deletions(-) create mode 100644 test/cryfs/filesystem/CryNodeTest.cpp create mode 100644 test/cryfs/filesystem/testutils/CryTestBase.h diff --git a/src/cryfs/filesystem/CryDevice.cpp b/src/cryfs/filesystem/CryDevice.cpp index e24aabea..50db4c76 100644 --- a/src/cryfs/filesystem/CryDevice.cpp +++ b/src/cryfs/filesystem/CryDevice.cpp @@ -205,4 +205,8 @@ void CryDevice::callFsActionCallbacks() const { } } +uint64_t CryDevice::numBlocks() const { + return _fsBlobStore->numBlocks(); +} + } diff --git a/src/cryfs/filesystem/CryDevice.h b/src/cryfs/filesystem/CryDevice.h index ed2b22a5..ebb02b1c 100644 --- a/src/cryfs/filesystem/CryDevice.h +++ b/src/cryfs/filesystem/CryDevice.h @@ -35,6 +35,8 @@ public: void callFsActionCallbacks() const; + uint64_t numBlocks() const; + private: cpputils::unique_ref _fsBlobStore; diff --git a/src/cryfs/filesystem/CryNode.cpp b/src/cryfs/filesystem/CryNode.cpp index f6bed7f5..eeb77ef1 100644 --- a/src/cryfs/filesystem/CryNode.cpp +++ b/src/cryfs/filesystem/CryNode.cpp @@ -64,8 +64,11 @@ void CryNode::rename(const bf::path &to) { } std::string oldName = old->name(); // Store, because if targetDir == *_parent, then the 'old' object will be invalidated after we add something to targetDir if (targetDir->key() != (*_parent)->key() || oldName != to.filename().native()) { + auto onOverwritten = [this] (const blockstore::Key &key) { + device()->RemoveBlob(key); + }; targetDir->AddOrOverwriteChild(to.filename().native(), old->key(), old->type(), old->mode(), old->uid(), old->gid(), - old->lastAccessTime(), old->lastModificationTime()); + old->lastAccessTime(), old->lastModificationTime(), onOverwritten); (*_parent)->RemoveChild(oldName); } } diff --git a/src/cryfs/filesystem/CryNode.h b/src/cryfs/filesystem/CryNode.h index fbb811d1..2b00cb39 100644 --- a/src/cryfs/filesystem/CryNode.h +++ b/src/cryfs/filesystem/CryNode.h @@ -12,6 +12,8 @@ namespace cryfs { class CryNode: public virtual fspp::Node { public: + virtual ~CryNode(); + CryNode(CryDevice *device, boost::optional> parent, const blockstore::Key &key); void access(int mask) const override; void stat(struct ::stat *result) const override; @@ -22,7 +24,6 @@ public: protected: CryNode(); - virtual ~CryNode(); CryDevice *device(); const CryDevice *device() const; diff --git a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h index a64cd780..15d0ab7f 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h @@ -43,8 +43,9 @@ public: } void AddOrOverwriteChild(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) { - return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime); + mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten) { + return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten); } void statChild(const blockstore::Key &key, struct ::stat *result) const { diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp index 4a9091c2..72e0e192 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp @@ -65,28 +65,32 @@ void DirBlob::_readEntriesFromBlob() { } void DirBlob::AddChildDir(const std::string &name, const Key &blobKey, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { + std::unique_lock lock(_mutex); _addChild(name, blobKey, fspp::Dir::EntryType::DIR, mode, uid, gid, lastAccessTime, lastModificationTime); } void DirBlob::AddChildFile(const std::string &name, const Key &blobKey, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { + std::unique_lock lock(_mutex); _addChild(name, blobKey, fspp::Dir::EntryType::FILE, mode, uid, gid, lastAccessTime, lastModificationTime); } void DirBlob::AddChildSymlink(const std::string &name, const blockstore::Key &blobKey, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { + std::unique_lock lock(_mutex); _addChild(name, blobKey, fspp::Dir::EntryType::SYMLINK, S_IFLNK | S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH, uid, gid, lastAccessTime, lastModificationTime); } void DirBlob::_addChild(const std::string &name, const Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); _entries.add(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); _changed = true; } -void DirBlob::AddOrOverwriteChild(const std::string &name, const Key &blobKey, - fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { +void DirBlob::AddOrOverwriteChild(const std::string &name, const Key &blobKey, fspp::Dir::EntryType entryType, + mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten) { std::unique_lock lock(_mutex); - _entries.addOrOverwrite(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); + + _entries.addOrOverwrite(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten); _changed = true; } diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.h b/src/cryfs/filesystem/fsblobstore/DirBlob.h index 28c0f476..4234fd96 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.h +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.h @@ -44,7 +44,8 @@ namespace cryfs { void AddChildSymlink(const std::string &name, const blockstore::Key &blobKey, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); void AddOrOverwriteChild(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); + mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten); void RemoveChild(const std::string &name); diff --git a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp index fbebc04b..13af750e 100644 --- a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp +++ b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.cpp @@ -61,16 +61,18 @@ void DirEntryList::_add(const string &name, const Key &blobKey, fspp::Dir::Entry } void DirEntryList::addOrOverwrite(const string &name, const Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, - uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { + uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten) { auto found = _findByName(name); if (found != _entries.end()) { - _overwrite(&*found, name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); + onOverwritten(found->key()); + _overwrite(found, name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); } else { _add(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); } } -void DirEntryList::_overwrite(DirEntry *entry, const string &name, const Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, +void DirEntryList::_overwrite(vector::iterator entry, const string &name, const Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { if (entry->type() != entryType) { if (entry->type() == fspp::Dir::EntryType::DIR) { @@ -82,7 +84,10 @@ void DirEntryList::_overwrite(DirEntry *entry, const string &name, const Key &bl throw fspp::fuse::FuseErrnoException(ENOTDIR); } } - *entry = DirEntry(entryType, name, blobKey, mode, uid, gid, lastAccessTime, lastModificationTime, time::now()); + // The new entry has possibly a different key, so it has to be in a different list position (list is ordered by keys). + // That's why we remove-and-add instead of just modifying the existing entry. + _entries.erase(entry); + _add(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime); } boost::optional DirEntryList::get(const string &name) const { diff --git a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h index e98a918b..0f0f1b79 100644 --- a/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h +++ b/src/cryfs/filesystem/fsblobstore/utils/DirEntryList.h @@ -24,7 +24,8 @@ namespace cryfs { void add(const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); void addOrOverwrite(const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType entryType, - mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); + mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten); boost::optional get(const std::string &name) const; boost::optional get(const blockstore::Key &key) const; void remove(const std::string &name); @@ -50,7 +51,7 @@ namespace cryfs { std::vector::iterator _findFirst(const blockstore::Key &hint, std::function pred); void _add(const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); - void _overwrite(DirEntry *entry, const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType entryType, + void _overwrite(std::vector::iterator entry, const std::string &name, const blockstore::Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime); std::vector _entries; diff --git a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h index d1097148..1498900e 100644 --- a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h @@ -39,8 +39,9 @@ public: } void AddOrOverwriteChild(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) { - return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime); + mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, + std::function onOverwritten) { + return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten); } void statChild(const blockstore::Key &key, struct ::stat *result) const { diff --git a/test/cryfs/CMakeLists.txt b/test/cryfs/CMakeLists.txt index 514ba306..c7be303b 100644 --- a/test/cryfs/CMakeLists.txt +++ b/test/cryfs/CMakeLists.txt @@ -15,6 +15,7 @@ set(SOURCES config/CryConfigLoaderTest.cpp config/CryConfigConsoleTest.cpp filesystem/CryFsTest.cpp + filesystem/CryNodeTest.cpp filesystem/FileSystemTest.cpp ) diff --git a/test/cryfs/filesystem/CryNodeTest.cpp b/test/cryfs/filesystem/CryNodeTest.cpp new file mode 100644 index 00000000..ffe7bad1 --- /dev/null +++ b/test/cryfs/filesystem/CryNodeTest.cpp @@ -0,0 +1,43 @@ +#include +#include "testutils/CryTestBase.h" +#include +#include +#include + +using cpputils::unique_ref; +using cpputils::dynamic_pointer_move; +using namespace cryfs; +namespace bf = boost::filesystem; + +// Many generic (black box) test cases for FsppNode are covered in Fspp fstest. +// This class adds some tests that need insight into how CryFS works. + +class CryNodeTest : public ::testing::Test, public CryTestBase { +public: + static constexpr mode_t MODE_PUBLIC = S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH; + + unique_ref CreateFile(const bf::path &path) { + auto _parentDir = device().Load(path.parent_path()).value(); + auto parentDir = dynamic_pointer_move(_parentDir).value(); + parentDir->createAndOpenFile(path.filename().native(), MODE_PUBLIC, 0, 0); + auto createdFile = device().Load(path).value(); + return dynamic_pointer_move(createdFile).value(); + } +}; + +TEST_F(CryNodeTest, Rename_DoesntLeaveBlocksOver) { + auto node = CreateFile("/oldname"); + EXPECT_EQ(2u, device().numBlocks()); // In the beginning, there is two blocks (the root block and the created file). If that is not true anymore, we'll have to adapt the test case. + node->rename("/newname"); + EXPECT_EQ(2u, device().numBlocks()); // Still same number of blocks +} + +// TODO Add similar test cases (i.e. checking number of blocks) for other situations in rename, and also for other operations (e.g. deleting files). + +TEST_F(CryNodeTest, Rename_Overwrite_DoesntLeaveBlocksOver) { + auto node = CreateFile("/oldname"); + CreateFile("/newexistingname"); + EXPECT_EQ(3u, device().numBlocks()); // In the beginning, there is three blocks (the root block and the two created files). If that is not true anymore, we'll have to adapt the test case. + node->rename("/newexistingname"); + EXPECT_EQ(2u, device().numBlocks()); // Only the blocks of one file are left +} diff --git a/test/cryfs/filesystem/testutils/CryTestBase.h b/test/cryfs/filesystem/testutils/CryTestBase.h new file mode 100644 index 00000000..a232d700 --- /dev/null +++ b/test/cryfs/filesystem/testutils/CryTestBase.h @@ -0,0 +1,33 @@ +#ifndef MESSMER_CRYFS_TEST_CRYFS_FILESYSTEM_CRYTESTBASE_H +#define MESSMER_CRYFS_TEST_CRYFS_FILESYSTEM_CRYTESTBASE_H + +#include +#include +#include +#include + +class CryTestBase { +public: + CryTestBase(): _configFile(false), _device(nullptr) { + auto fakeBlockStore = cpputils::make_unique_ref(); + _device = std::make_unique(configFile(), std::move(fakeBlockStore)); + } + + cryfs::CryConfigFile configFile() { + cryfs::CryConfig config; + config.SetCipher("aes-256-gcm"); + config.SetEncryptionKey(cpputils::AES256_GCM::CreateKey(cpputils::Random::PseudoRandom()).ToString()); + config.SetBlocksizeBytes(10240); + return cryfs::CryConfigFile::create(_configFile.path(), std::move(config), "mypassword", cpputils::SCrypt::TestSettings); + } + + cryfs::CryDevice &device() { + return *_device; + } + +private: + cpputils::TempFile _configFile; + std::unique_ptr _device; +}; + +#endif