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

@@ -205,4 +205,8 @@ void CryDevice::callFsActionCallbacks() const {
   }
 }
 
+uint64_t CryDevice::numBlocks() const {
+  return _fsBlobStore->numBlocks();
+}
+
 }

src/cryfs/filesystem/CryDevice.h

@@ -35,6 +35,8 @@ public:
 
   void callFsActionCallbacks() const;
 
+  uint64_t numBlocks() const;
+
 private:
 
   cpputils::unique_ref<parallelaccessfsblobstore::ParallelAccessFsBlobStore> _fsBlobStore;

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);
   }
 }

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<cpputils::unique_ref<parallelaccessfsblobstore::DirBlobRef>> 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;

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) {
+                  mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime,
-        return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime);
+                  std::function<void (const blockstore::Key &key)> onOverwritten) {
+        return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten);
     }
 
     void statChild(const blockstore::Key &key, struct ::stat *result) const {

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(_mutex);
   _entries.add(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime);
   _changed = true;
 }
 
-void DirBlob::AddOrOverwriteChild(const std::string &name, const Key &blobKey,
+void DirBlob::AddOrOverwriteChild(const std::string &name, const Key &blobKey, fspp::Dir::EntryType entryType,
-                       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<void (const blockstore::Key &key)> onOverwritten) {
   std::unique_lock<std::mutex> 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;
 }
 

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<void (const blockstore::Key &key)> onOverwritten);
 
             void RemoveChild(const std::string &name);
 

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<void (const blockstore::Key &key)> 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<DirEntry>::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<const DirEntry&> DirEntryList::get(const string &name) const {

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<void (const blockstore::Key &key)> onOverwritten);
             boost::optional<const DirEntry&> get(const std::string &name) const;
             boost::optional<const DirEntry&> get(const blockstore::Key &key) const;
             void remove(const std::string &name);
@@ -50,7 +51,7 @@ namespace cryfs {
             std::vector<DirEntry>::iterator _findFirst(const blockstore::Key &hint, std::function<bool (const DirEntry&)> 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<DirEntry>::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<DirEntry> _entries;

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) {
+                  mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime,
-        return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime);
+                  std::function<void (const blockstore::Key &key)> onOverwritten) {
+        return _base->AddOrOverwriteChild(name, blobKey, type, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten);
     }
 
     void statChild(const blockstore::Key &key, struct ::stat *result) const {

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
 )
 

test/cryfs/filesystem/CryNodeTest.cpp

@@ -0,0 +1,43 @@
+#include <gtest/gtest.h>
+#include "testutils/CryTestBase.h"
+#include <cryfs/filesystem/CryDir.h>
+#include <cryfs/filesystem/CryFile.h>
+#include <cryfs/filesystem/CryOpenFile.h>
+
+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<CryNode> CreateFile(const bf::path &path) {
+        auto _parentDir = device().Load(path.parent_path()).value();
+        auto parentDir = dynamic_pointer_move<CryDir>(_parentDir).value();
+        parentDir->createAndOpenFile(path.filename().native(), MODE_PUBLIC, 0, 0);
+        auto createdFile = device().Load(path).value();
+        return dynamic_pointer_move<CryNode>(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
+}

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 <cryfs/filesystem/CryDevice.h>
+#include <blockstore/implementations/testfake/FakeBlockStore.h>
+#include <cpp-utils/tempfile/TempFile.h>
+#include <cpp-utils/crypto/kdf/Scrypt.h>
+
+class CryTestBase {
+public:
+    CryTestBase(): _configFile(false), _device(nullptr) {
+        auto fakeBlockStore = cpputils::make_unique_ref<blockstore::testfake::FakeBlockStore>();
+        _device = std::make_unique<cryfs::CryDevice>(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<cryfs::CryDevice> _device;
+};
+
+#endif