Fix small rename corner case and add test cases for timestamps in many rename corner cases

src/cryfs/filesystem/CryNode.cpp

@@ -72,6 +72,8 @@ void CryNode::rename(const bf::path &to) {
     targetDir->AddOrOverwriteChild(to.filename().native(), oldEntry.key(), oldEntry.type(), oldEntry.mode(), oldEntry.uid(), oldEntry.gid(),
                                    oldEntry.lastAccessTime(), oldEntry.lastModificationTime(), onOverwritten);
     (*_parent)->RemoveChild(oldEntry.name());
+    // targetDir is now the new parent for this node. Adapt to it, so we can call further operations on this node object.
+    _parent = cpputils::to_unique_ptr(std::move(targetDir));
   }
 }
 

src/fspp/fstest/FsppNodeTest_Rename.h

@@ -8,7 +8,7 @@
 template<class ConcreteFileSystemTestFixture>
 class FsppNodeTest_Rename: public FsppNodeTest<ConcreteFileSystemTestFixture> {
 public:
-    void Test_TargetParentDirDoesntExist() {
+    void Test_Error_TargetParentDirDoesntExist() {
         auto node = this->CreateNode("/oldname");
         try {
             node->rename("/notexistingdir/newname");
@@ -20,7 +20,7 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/oldname"));
     }
 
-    void Test_TargetParentDirIsFile() {
+    void Test_Error_TargetParentDirIsFile() {
         auto node = this->CreateNode("/oldname");
         this->CreateFile("/somefile");
         try {
@@ -34,6 +34,16 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/somefile"));
     }
 
+    void Test_Error_RootDir() {
+        auto root = this->LoadDir("/");
+        try {
+            root->rename("/newname");
+            EXPECT_TRUE(false); // expect throws
+        } catch (const fspp::fuse::FuseErrnoException &e) {
+            EXPECT_EQ(EBUSY, e.getErrno());
+        }
+    }
+
     void Test_InRoot() {
         auto node = this->CreateNode("/oldname");
         node->rename("/newname");
@@ -105,16 +115,6 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/oldname"));
     }
 
-    void Test_RootDir() {
-        auto root = this->LoadDir("/");
-        try {
-            root->rename("/newname");
-            EXPECT_TRUE(false); // expect throws
-        } catch (const fspp::fuse::FuseErrnoException &e) {
-            EXPECT_EQ(EBUSY, e.getErrno());
-        }
-    }
-
     void Test_Overwrite_InSameDir() {
         auto node = this->CreateNode("/oldname");
         this->CreateNode("/newname");
@@ -141,7 +141,7 @@ public:
         EXPECT_EQ(3u, this->LoadDir("/")->children()->size()); // 3, because of '.' and '..'
     }
 
-    void Test_Overwrite_DirWithFile_InSameDir() {
+    void Test_Overwrite_Error_DirWithFile_InSameDir() {
         auto file = this->CreateFile("/oldname");
         this->CreateDir("/newname");
         try {
@@ -154,7 +154,7 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/newname"));
     }
 
-    void Test_Overwrite_DirWithFile_InDifferentDir() {
+    void Test_Overwrite_Error_DirWithFile_InDifferentDir() {
         this->CreateDir("/parent1");
         this->CreateDir("/parent2");
         auto file = this->CreateFile("/parent1/oldname");
@@ -169,7 +169,7 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/parent2/newname"));
     }
 
-    void Test_Overwrite_FileWithDir_InSameDir() {
+    void Test_Overwrite_Error_FileWithDir_InSameDir() {
         auto dir = this->CreateDir("/oldname");
         this->CreateFile("/newname");
         try {
@@ -182,7 +182,7 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/newname"));
     }
 
-    void Test_Overwrite_FileWithDir_InDifferentDir() {
+    void Test_Overwrite_Error_FileWithDir_InDifferentDir() {
         this->CreateDir("/parent1");
         this->CreateDir("/parent2");
         auto dir = this->CreateDir("/parent1/oldname");
@@ -196,11 +196,24 @@ public:
         EXPECT_NE(boost::none, this->device->Load("/parent1/oldname"));
         EXPECT_NE(boost::none, this->device->Load("/parent2/newname"));
     }
+
+    void Test_CanRenameTwice() {
+        // Test that the node object stays valid after a rename, even if it now points to an entry of a different parent directory.
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        auto node = this->CreateNode("/oldname");
+        node->rename("/mydir1/newname");
+        node->rename("/mydir2/newname");
+        EXPECT_EQ(boost::none, this->device->Load("/oldname"));
+        EXPECT_EQ(boost::none, this->device->Load("/mydir1/newname"));
+        EXPECT_NE(boost::none, this->device->Load("/mydir2/newname"));
+    }
 };
 
 REGISTER_NODE_TEST_CASE(FsppNodeTest_Rename,
-    TargetParentDirDoesntExist,
+    Error_TargetParentDirDoesntExist,
-    TargetParentDirIsFile,
+    Error_TargetParentDirIsFile,
+    Error_RootDir,
     InRoot,
     InNested,
     RootToNested_SameName,
@@ -210,14 +223,14 @@ REGISTER_NODE_TEST_CASE(FsppNodeTest_Rename,
     NestedToNested_SameName,
     NestedToNested_NewName,
     ToItself,
-    RootDir,
     Overwrite_InSameDir,
     Overwrite_InDifferentDir,
     Overwrite_DoesntHaveSameEntryTwice,
-    Overwrite_DirWithFile_InSameDir,
+    Overwrite_Error_DirWithFile_InSameDir,
-    Overwrite_DirWithFile_InDifferentDir,
+    Overwrite_Error_DirWithFile_InDifferentDir,
-    Overwrite_FileWithDir_InSameDir,
+    Overwrite_Error_FileWithDir_InSameDir,
-    Overwrite_FileWithDir_InDifferentDir
+    Overwrite_Error_FileWithDir_InDifferentDir,
+    CanRenameTwice
 );
 
 #endif

src/fspp/fstest/FsppNodeTest_Timestamps.h

@@ -62,17 +62,209 @@ public:
         EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
     }
 
-    void Test_Rename() {
+    void Test_Rename_Error_TargetParentDirDoesntExist() {
-        auto node = this->CreateNode("/mynode");
+        auto node = this->CreateNode("/oldname");
         auto operation = [&node] () {
-            node->rename("newnodename");
+            try {
+                node->rename("/notexistingdir/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(ENOENT, e.getErrno()); //Rename fails, everything is ok.
+            }
         };
-        EXPECT_OPERATION_DOESNT_UPDATE_ACCESS_TIMESTAMP(*node, operation);
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
-        EXPECT_OPERATION_DOESNT_UPDATE_MODIFICATION_TIMESTAMP(*node, operation);
-        EXPECT_OPERATION_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
     }
 
-    // TODO Other rename cases (e.g. failed renames/error paths, moving to different dir, ...) from FsppNodeTest_Rename
+    void Test_Rename_Error_TargetParentDirIsFile() {
+        auto node = this->CreateNode("/oldname");
+        this->CreateFile("/somefile");
+        auto operation = [&node] () {
+            try {
+                node->rename("/somefile/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(ENOTDIR, e.getErrno()); //Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
+    }
+
+    void Test_Rename_Error_RootDir() {
+        // TODO Re-enable this test once the root dir stores timestamps correctly
+        /*
+        auto root = this->LoadDir("/");
+        auto operation = [&root] () {
+            try {
+                root->rename("/newname");
+                EXPECT_TRUE(false); // expect throws
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(EBUSY, e.getErrno()); //Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*root, operation);
+         */
+    }
+
+    void Test_Rename_InRoot() {
+        auto node = this->CreateNode("/oldname");
+        auto operation = [&node] () {
+            node->rename("/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_InNested() {
+        this->CreateDir("/mydir");
+        auto node = this->CreateNode("/mydir/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_RootToNested_SameName() {
+        this->CreateDir("/mydir");
+        auto node = this->CreateNode("/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir/oldname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_RootToNested_NewName() {
+        this->CreateDir("/mydir");
+        auto node = this->CreateNode("/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_NestedToRoot_SameName() {
+        this->CreateDir("/mydir");
+        auto node = this->CreateNode("/mydir/oldname");
+        auto operation = [&node] () {
+            node->rename("/oldname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_NestedToRoot_NewName() {
+        this->CreateDir("/mydir");
+        auto node = this->CreateNode("/mydir/oldname");
+        auto operation = [&node] () {
+            node->rename("/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_NestedToNested_SameName() {
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        auto node = this->CreateNode("/mydir1/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir2/oldname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_NestedToNested_NewName() {
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        auto node = this->CreateNode("/mydir1/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir2/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_ToItself() {
+        auto node = this->CreateNode("/oldname");
+        auto operation = [&node] () {
+            node->rename("/oldname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_InSameDir() {
+        auto node = this->CreateNode("/oldname");
+        this->CreateNode("/newname");
+        auto operation = [&node] () {
+            node->rename("/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_InDifferentDir() {
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        this->CreateNode("/mydir2/newname");
+        auto node = this->CreateNode("/mydir1/oldname");
+        auto operation = [&node] () {
+            node->rename("/mydir2/newname");
+        };
+        EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_Error_DirWithFile_InSameDir() {
+        auto node = this->CreateFile("/oldname");
+        this->CreateDir("/newname");
+        auto operation = [&node] () {
+            try {
+                node->rename("/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(EISDIR, e.getErrno()); //Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_Error_DirWithFile_InDifferentDir() {
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        auto node = this->CreateFile("/mydir1/oldname");
+        this->CreateDir("/mydir2/newname");
+        auto operation = [&node] () {
+            try {
+                node->rename("/mydir2/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(EISDIR, e.getErrno());//Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_Error_FileWithDir_InSameDir() {
+        auto node = this->CreateDir("/oldname");
+        this->CreateFile("/newname");
+        auto operation = [&node] () {
+            try {
+                node->rename("/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(ENOTDIR, e.getErrno()); //Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
+    }
+
+    void Test_Rename_Overwrite_Error_FileWithDir_InDifferentDir() {
+        this->CreateDir("/mydir1");
+        this->CreateDir("/mydir2");
+        auto node = this->CreateDir("/mydir1/oldname");
+        this->CreateFile("/mydir2/newname");
+        auto operation = [&node] () {
+            try {
+                node->rename("/mydir2/newname");
+                EXPECT_TRUE(false); // expect rename to fail
+            } catch (const fspp::fuse::FuseErrnoException &e) {
+                EXPECT_EQ(ENOTDIR, e.getErrno()); //Rename fails, everything is ok.
+            }
+        };
+        EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(*node, operation);
+    }
 
     void Test_Utimens() {
         auto node = this->CreateNode("/mynode");
@@ -93,7 +285,24 @@ REGISTER_NODE_TEST_CASE(FsppNodeTest_Timestamps,
     Chmod,
     Chown,
     Access,
-    Rename,
+    Rename_Error_TargetParentDirDoesntExist,
+    Rename_Error_TargetParentDirIsFile,
+    Rename_Error_RootDir,
+    Rename_InRoot,
+    Rename_InNested,
+    Rename_RootToNested_SameName,
+    Rename_RootToNested_NewName,
+    Rename_NestedToRoot_SameName,
+    Rename_NestedToRoot_NewName,
+    Rename_NestedToNested_SameName,
+    Rename_NestedToNested_NewName,
+    Rename_ToItself,
+    Rename_Overwrite_InSameDir,
+    Rename_Overwrite_InDifferentDir,
+    Rename_Overwrite_Error_DirWithFile_InSameDir,
+    Rename_Overwrite_Error_DirWithFile_InDifferentDir,
+    Rename_Overwrite_Error_FileWithDir_InSameDir,
+    Rename_Overwrite_Error_FileWithDir_InDifferentDir,
     Utimens
 );
 

src/fspp/fstest/testutils/TimestampTestUtils.h

@@ -70,9 +70,34 @@ public:
     }
 
     void EXPECT_OPERATION_DOESNT_UPDATE_TIMESTAMPS(const fspp::Node &node, std::function<void()> operation) {
-        EXPECT_OPERATION_DOESNT_UPDATE_ACCESS_TIMESTAMP(node, operation);
+        // equivalent to the following, but implemented separately because operation() should only be called once.
-        EXPECT_OPERATION_DOESNT_UPDATE_MODIFICATION_TIMESTAMP(node, operation);
+        // EXPECT_OPERATION_DOESNT_UPDATE_ACCESS_TIMESTAMP(node, operation);
-        EXPECT_OPERATION_DOESNT_UPDATE_METADATACHANGE_TIMESTAMP(node, operation);
+        // EXPECT_OPERATION_DOESNT_UPDATE_MODIFICATION_TIMESTAMP(node, operation);
+        // EXPECT_OPERATION_DOESNT_UPDATE_METADATACHANGE_TIMESTAMP(node, operation);
+        ensureNodeTimestampsAreOld(node);
+        struct stat oldStat = stat(node);
+        operation();
+        struct stat newStat = stat(node);
+        EXPECT_EQ(oldStat.st_atim, newStat.st_atim);
+        EXPECT_EQ(oldStat.st_mtim, newStat.st_mtim);
+        EXPECT_LE(oldStat.st_ctim, newStat.st_ctim);
+    }
+
+    void EXPECT_OPERATION_ONLY_UPDATES_METADATACHANGE_TIMESTAMP(const fspp::Node &node, std::function<void()> operation) {
+        // equivalent to the following, but implemented separately because operation() should only be called once.
+        // EXPECT_OPERATION_DOESNT_UPDATE_ACCESS_TIMESTAMP(node, operation);
+        // EXPECT_OPERATION_DOESNT_UPDATE_MODIFICATION_TIMESTAMP(node, operation);
+        // EXPECT_OPERATION_UPDATES_METADATACHANGE_TIMESTAMP(node, operation);
+        ensureNodeTimestampsAreOld(node);
+        struct stat oldStat = stat(node);
+        timespec lowerBound = cpputils::time::now();
+        operation();
+        timespec upperBound = cpputils::time::now();
+        struct stat newStat = stat(node);
+        EXPECT_EQ(oldStat.st_atim, newStat.st_atim);
+        EXPECT_EQ(oldStat.st_mtim, newStat.st_mtim);
+        EXPECT_LE(lowerBound, newStat.st_ctim);
+        EXPECT_GE(upperBound, newStat.st_ctim);
     }
 
     struct stat stat(const fspp::Node &node) {