diff --git a/files/localfilesystem.cpp b/files/localfilesystem.cpp index 1545ba9..df06247 100644 --- a/files/localfilesystem.cpp +++ b/files/localfilesystem.cpp @@ -321,6 +321,21 @@ static void validateDestination(const std::string& source, const std::string& de throw Failure(EINVAL, destination); } +// A folder that holds the source cannot be replaced: merging into it would write the +// source's copy into the source itself, and a move would then delete both. +static void validateReplacement(const std::string& source, const std::string& destination) +{ + const std::string sourceParent = canonicalPath(absolutePathOf(source)); + const std::string destinationParent = canonicalPath(absolutePathOf(destination)); + if (sourceParent.empty() || destinationParent.empty()) + return; + + const std::string src = joined(sourceParent, fileNameOf(source)); + const std::string dst = joined(destinationParent, fileNameOf(destination)); + if (src.starts_with(dst + '/')) + throw Failure(EINVAL, destination); +} + static void mergeStaged(int source, int target, const std::string& path, OperationContext& ctx) { children(source, path, [&](const std::string& name, const std::string& childPath) { @@ -395,6 +410,8 @@ static std::string publish(int parent, const std::string& temporary, const std:: throw Failure(EEXIST, destination); if (policy == ConflictPolicy::Replace) { + validateReplacement(source, destination); + // Copies are not synced one by one; the replacement must be on disk before the // original goes. check(::syncfs(parent), destination); @@ -517,6 +534,7 @@ std::string LocalFileSystem::move(const std::string& source, const std::string& } if (code == EEXIST && policy == ConflictPolicy::Replace) { + validateReplacement(source, destination); if (::renameat(input.fd.value, input.name.c_str(), output.fd.value, output.name.c_str()) == 0) return destination; if (errno != EXDEV && errno != ENOTEMPTY && errno != EEXIST) diff --git a/files/tests/files_test.cpp b/files/tests/files_test.cpp index 20ddfd8..d42c24c 100644 --- a/files/tests/files_test.cpp +++ b/files/tests/files_test.cpp @@ -389,6 +389,38 @@ TEST_F(FilesTest, SelfAndDescendantProtection) EXPECT_EQ(errorCode([&] { LocalFileSystem::remove("/", ctx); }), EINVAL); } +TEST_F(FilesTest, ReplacingAFolderThatHoldsTheSourceIsRefused) +{ + TemporaryDirectory dir; + ASSERT_TRUE(makePath(dir.filePath("b/b/b"))); + writeFile(dir.filePath("b/b/b/inner"), "inner"); + writeFile(dir.filePath("b/b/file"), "file"); + writeFile(dir.filePath("b/keep"), "keep"); + + OperationContext ctx; + EXPECT_EQ(errorCode([&] { + LocalFileSystem::copy(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::Replace, ctx); + }), + EINVAL); + + EXPECT_EQ(errorCode([&] { + LocalFileSystem::move(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::Replace, ctx); + }), + EINVAL); + + EXPECT_EQ(readFile(dir.filePath("b/b/b/inner")), "inner"); + EXPECT_EQ(readFile(dir.filePath("b/b/file")), "file"); + EXPECT_EQ(readFile(dir.filePath("b/keep")), "keep"); + EXPECT_FALSE(exists(dir.filePath("b/file"))); + + EXPECT_EQ(LocalFileSystem::move(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::KeepBoth, ctx), + dir.filePath("b (1)")); + EXPECT_EQ(readFile(dir.filePath("b (1)/b/inner")), "inner"); + + for (const std::string& name : entries(dir.path())) + EXPECT_FALSE(name.starts_with(".cutefish-")) << name; +} + TEST_F(FilesTest, DestinationSymlinkIsNotFollowed) { TemporaryDirectory dir;