From 7feabcd09690dc4ebed4f32389ea31b6025563a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?L=C3=A9o=20Lam?= <leo@innovatetechnologi.es>
Date: Tue, 8 May 2018 23:55:13 +0200
Subject: [PATCH] IOS/FS: Fix rename not handling existing target correctly

The existing backend did not handle cases where the target exists
correctly.

This is a bug that has been around forever but was only recently
exposed when ES started to use our FS code.

Also adds some unit tests to make sure this won't get broken again.
---
 Source/Core/Core/IOS/FS/HostBackend/FS.cpp    | 13 +++--
 .../UnitTests/Core/IOS/FS/FileSystemTest.cpp  | 51 +++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/Source/Core/Core/IOS/FS/HostBackend/FS.cpp b/Source/Core/Core/IOS/FS/HostBackend/FS.cpp
index 112eee0e66..19479bf318 100644
--- a/Source/Core/Core/IOS/FS/HostBackend/FS.cpp
+++ b/Source/Core/Core/IOS/FS/HostBackend/FS.cpp
@@ -232,10 +232,17 @@ ResultCode HostFileSystem::Rename(Uid, Gid, const std::string& old_path,
   // try to make the basis directory
   File::CreateFullPath(new_name);
 
-  // if there is already a file, delete it
-  if (File::Exists(old_name) && File::Exists(new_name))
+  // If there is already something of the same type at the new path, delete it.
+  if (File::Exists(new_name))
   {
-    File::Delete(new_name);
+    const bool old_is_file = File::IsFile(old_name);
+    const bool new_is_file = File::IsFile(new_name);
+    if (old_is_file && new_is_file)
+      File::Delete(new_name);
+    else if (!old_is_file && !new_is_file)
+      File::DeleteDirRecursively(new_name);
+    else
+      return ResultCode::Invalid;
   }
 
   // finally try to rename the file
diff --git a/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp b/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp
index cc6dd25b91..9964813a7d 100644
--- a/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp
+++ b/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp
@@ -107,6 +107,57 @@ TEST_F(FileSystemTest, Rename)
   EXPECT_TRUE(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/test").Succeeded());
 }
 
+TEST_F(FileSystemTest, RenameWithExistingTargetDirectory)
+{
+  // Test directory -> existing, non-empty directory.
+  // IOS's FS sysmodule is not POSIX compliant and will remove the existing directory
+  // if it exists, even when there are files in it.
+  ASSERT_EQ(
+      m_fs->CreateDirectory(Uid{0}, Gid{0}, "/tmp/d", 0, Mode::ReadWrite, Mode::None, Mode::None),
+      ResultCode::Success);
+  ASSERT_EQ(
+      m_fs->CreateDirectory(Uid{0}, Gid{0}, "/tmp/d2", 0, Mode::ReadWrite, Mode::None, Mode::None),
+      ResultCode::Success);
+  ASSERT_EQ(
+      m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/d2/file", 0, Mode::ReadWrite, Mode::None, Mode::None),
+      ResultCode::Success);
+  EXPECT_EQ(m_fs->Rename(Uid{0}, Gid{0}, "/tmp/d", "/tmp/d2"), ResultCode::Success);
+
+  EXPECT_EQ(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp/d").Error(), ResultCode::NotFound);
+  const Result<std::vector<std::string>> children = m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp/d2");
+  ASSERT_TRUE(children.Succeeded());
+  EXPECT_TRUE(children->empty());
+}
+
+TEST_F(FileSystemTest, RenameWithExistingTargetFile)
+{
+  // Create the test source file and write some data (so that we can check its size later on).
+  ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f1", 0, Mode::ReadWrite, Mode::None, Mode::None),
+            ResultCode::Success);
+  const std::vector<u8> TEST_DATA{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}};
+  std::vector<u8> read_buffer(TEST_DATA.size());
+  {
+    const Result<FileHandle> file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f1", Mode::ReadWrite);
+    ASSERT_TRUE(file.Succeeded());
+    ASSERT_TRUE(file->Write(TEST_DATA.data(), TEST_DATA.size()).Succeeded());
+  }
+
+  // Create the test target file and leave it empty.
+  ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f2", 0, Mode::ReadWrite, Mode::None, Mode::None),
+            ResultCode::Success);
+
+  // Rename f1 to f2 and check that f1 replaced f2.
+  EXPECT_EQ(m_fs->Rename(Uid{0}, Gid{0}, "/tmp/f1", "/tmp/f2"), ResultCode::Success);
+
+  ASSERT_FALSE(m_fs->GetMetadata(Uid{0}, Gid{0}, "/tmp/f1").Succeeded());
+  EXPECT_EQ(m_fs->GetMetadata(Uid{0}, Gid{0}, "/tmp/f1").Error(), ResultCode::NotFound);
+
+  const Result<Metadata> metadata = m_fs->GetMetadata(Uid{0}, Gid{0}, "/tmp/f2");
+  ASSERT_TRUE(metadata.Succeeded());
+  EXPECT_TRUE(metadata->is_file);
+  EXPECT_EQ(metadata->size, TEST_DATA.size());
+}
+
 TEST_F(FileSystemTest, GetDirectoryStats)
 {
   auto check_stats = [this](u32 clusters, u32 inodes) {