IOS/FS: Add a scoped FD class to make it harder to leak FDs

This changes FileSystemProxy::Open to return a file descriptor wrapper
that will ensure the FD is closed when it goes out of scope.

By using such a wrapper we make it more difficult to forget to close
file descriptors.

This fixes a leak in ReadBootContent. I should have added such a class
from the beginning... In practice, I don't think this would have caused
any obvious issue because ReadBootContent is only called after an IOS
relaunch -- which clears all FDs -- and most titles do not get close
to the FD limit.

Source/Core/Core/IOS/ES/ES.cpp

@@ -403,9 +403,9 @@ bool ESDevice::LaunchPPCTitle(u64 title_id)
   // To keep track of the PPC title launch, a temporary launch file (LAUNCH_FILE_PATH) is used
   // to store the title ID of the title to launch and its TMD.
   // The launch file not existing means an IOS reload is required.
-  const auto launch_file_fd = m_ios.GetFSDevice()->Open(PID_KERNEL, PID_KERNEL, LAUNCH_FILE_PATH,
+  if (const auto launch_file_fd = m_ios.GetFSDevice()->Open(
-                                                        FS::Mode::Read, {}, &ticks);
+          PID_KERNEL, PID_KERNEL, LAUNCH_FILE_PATH, FS::Mode::Read, {}, &ticks);
-  if (launch_file_fd < 0)
+      launch_file_fd.Get() < 0)
   {
     if (WriteLaunchFile(tmd, &ticks) != IPC_SUCCESS)
     {
@@ -423,7 +423,6 @@ bool ESDevice::LaunchPPCTitle(u64 title_id)
 
   // Otherwise, assume that the PPC title can now be launched directly.
   // Unlike IOS, we won't bother checking the title ID in the launch file. (It's not useful.)
-  m_ios.GetFSDevice()->Close(launch_file_fd, &ticks);
   m_ios.GetFSDevice()->DeleteFile(PID_KERNEL, PID_KERNEL, LAUNCH_FILE_PATH, &ticks);
   WriteSystemFile(SPACE_FILE_PATH, std::vector<u8>(SPACE_FILE_SIZE), &ticks);
 

Source/Core/Core/IOS/ES/Formats.cpp

@@ -528,15 +528,15 @@ SharedContentMap::SharedContentMap(std::shared_ptr<HLE::FSDevice> fs)
   static_assert(sizeof(Entry) == 28, "SharedContentMap::Entry has the wrong size");
 
   Entry entry;
-  s64 fd = fs->Open(PID_KERNEL, PID_KERNEL, CONTENT_MAP_PATH, HLE::FS::Mode::Read, {}, &m_ticks);
+  const auto fd =
-  if (fd < 0)
+      fs->Open(PID_KERNEL, PID_KERNEL, CONTENT_MAP_PATH, HLE::FS::Mode::Read, {}, &m_ticks);
+  if (fd.Get() < 0)
     return;
-  while (fs->Read(fd, &entry, 1, &m_ticks) == sizeof(entry))
+  while (fs->Read(fd.Get(), &entry, 1, &m_ticks) == sizeof(entry))
   {
     m_entries.push_back(entry);
     m_last_id++;
   }
-  fs->Close(fd, &m_ticks);
 }
 
 SharedContentMap::~SharedContentMap() = default;
@@ -621,18 +621,18 @@ static std::pair<u32, u64> ReadUidSysEntry(HLE::FSDevice& fs, u64 fd, u64* ticks
 constexpr char UID_MAP_PATH[] = "/sys/uid.sys";
 UIDSys::UIDSys(std::shared_ptr<HLE::FSDevice> fs) : m_fs_device{fs}, m_fs{fs->GetFS()}
 {
-  s64 fd = fs->Open(PID_KERNEL, PID_KERNEL, UID_MAP_PATH, HLE::FS::Mode::Read, {}, &m_ticks);
+  if (const auto fd =
-  if (fd >= 0)
+          fs->Open(PID_KERNEL, PID_KERNEL, UID_MAP_PATH, HLE::FS::Mode::Read, {}, &m_ticks);
+      fd.Get() >= 0)
   {
     while (true)
     {
-      std::pair<u32, u64> entry = ReadUidSysEntry(*fs, fd, &m_ticks);
+      std::pair<u32, u64> entry = ReadUidSysEntry(*fs, fd.Get(), &m_ticks);
       if (!entry.first && !entry.second)
         break;
 
       m_entries.insert(std::move(entry));
     }
-    fs->Close(fd, &m_ticks);
   }
 
   if (m_entries.empty())

Source/Core/Core/IOS/ES/NandUtils.cpp

@@ -27,13 +27,12 @@ namespace IOS::HLE
 {
 static ES::TMDReader FindTMD(FSDevice& fs, const std::string& tmd_path, Ticks ticks)
 {
-  const s64 fd = fs.Open(PID_KERNEL, PID_KERNEL, tmd_path, FS::Mode::Read, {}, ticks);
+  const auto fd = fs.Open(PID_KERNEL, PID_KERNEL, tmd_path, FS::Mode::Read, {}, ticks);
-  if (fd < 0)
+  if (fd.Get() < 0)
     return {};
-  Common::ScopeGuard guard{[&] { fs.Close(fd, ticks); }};
 
-  std::vector<u8> tmd_bytes(fs.GetFileStatus(fd, ticks)->size);
+  std::vector<u8> tmd_bytes(fs.GetFileStatus(fd.Get(), ticks)->size);
-  if (!fs.Read(fd, tmd_bytes.data(), tmd_bytes.size(), ticks))
+  if (!fs.Read(fd.Get(), tmd_bytes.data(), tmd_bytes.size(), ticks))
     return {};
 
   return ES::TMDReader{std::move(tmd_bytes)};
@@ -407,20 +406,20 @@ s32 ESDevice::WriteSystemFile(const std::string& path, const std::vector<u8>& da
     return FS::ConvertResult(result);
   }
 
-  const auto fd = fs.Open(PID_KERNEL, PID_KERNEL, tmp_path, FS::Mode::ReadWrite, {}, ticks);
+  auto fd = fs.Open(PID_KERNEL, PID_KERNEL, tmp_path, FS::Mode::ReadWrite, {}, ticks);
-  if (fd < 0)
+  if (fd.Get() < 0)
   {
-    ERROR_LOG_FMT(IOS_ES, "Failed to open temporary file {}: {}", tmp_path, fd);
+    ERROR_LOG_FMT(IOS_ES, "Failed to open temporary file {}: {}", tmp_path, fd.Get());
-    return fd;
+    return fd.Get();
   }
 
-  if (fs.Write(fd, data.data(), u32(data.size()), {}, ticks) != s32(data.size()))
+  if (fs.Write(fd.Get(), data.data(), u32(data.size()), {}, ticks) != s32(data.size()))
   {
     ERROR_LOG_FMT(IOS_ES, "Failed to write to temporary file {}", tmp_path);
     return ES_EIO;
   }
 
-  if (const auto ret = fs.Close(fd, ticks); ret != IPC_SUCCESS)
+  if (const auto ret = fs.Close(fd.Release(), ticks); ret != IPC_SUCCESS)
   {
     ERROR_LOG_FMT(IOS_ES, "Failed to close temporary file {}", tmp_path);
     return ret;

Source/Core/Core/IOS/ES/TitleContents.cpp

@@ -29,12 +29,12 @@ s32 ESDevice::OpenContent(const ES::TMDReader& tmd, u16 content_index, u32 uid,
       continue;
 
     const std::string path = GetContentPath(title_id, content, ticks);
-    s64 fd = m_ios.GetFSDevice()->Open(PID_KERNEL, PID_KERNEL, path, FS::Mode::Read, {}, ticks);
+    auto fd = m_ios.GetFSDevice()->Open(PID_KERNEL, PID_KERNEL, path, FS::Mode::Read, {}, ticks);
-    if (fd < 0)
+    if (fd.Get() < 0)
-      return fd;
+      return fd.Get();
 
     entry.m_opened = true;
-    entry.m_fd = fd;
+    entry.m_fd = fd.Release();
     entry.m_content = content;
     entry.m_title_id = title_id;
     entry.m_uid = uid;

Source/Core/Core/IOS/FS/FileSystemProxy.cpp

@@ -160,27 +160,28 @@ std::optional<IPCReply> FSDevice::Open(const OpenRequest& request)
 {
   return MakeIPCReply([&](Ticks t) {
     return Open(request.uid, request.gid, request.path, static_cast<Mode>(request.flags & 3),
-                request.fd, t);
+                request.fd, t)
+        .Release();
   });
 }
 
-s64 FSDevice::Open(FS::Uid uid, FS::Gid gid, const std::string& path, FS::Mode mode,
+FSDevice::ScopedFd FSDevice::Open(FS::Uid uid, FS::Gid gid, const std::string& path, FS::Mode mode,
-                   std::optional<u32> ipc_fd, Ticks ticks)
+                                  std::optional<u32> ipc_fd, Ticks ticks)
 {
   ticks.Add(IPC_OVERHEAD_TICKS);
 
   if (m_fd_map.size() >= 16)
-    return ConvertResult(ResultCode::NoFreeHandle);
+    return {this, ConvertResult(ResultCode::NoFreeHandle), ticks};
 
   if (path.size() >= 64)
-    return ConvertResult(ResultCode::Invalid);
+    return {this, ConvertResult(ResultCode::Invalid), ticks};
 
   const u64 fd = ipc_fd.has_value() ? u64(*ipc_fd) : m_next_fd++;
 
   if (path == "/dev/fs")
   {
     m_fd_map[fd] = {gid, uid, INVALID_FD};
-    return fd;
+    return {this, static_cast<s64>(fd), ticks};
   }
 
   ticks.Add(EstimateFileLookupTicks(path, FileLookupMode::Normal));
@@ -188,11 +189,11 @@ s64 FSDevice::Open(FS::Uid uid, FS::Gid gid, const std::string& path, FS::Mode m
   auto backend_fd = m_ios.GetFS()->OpenFile(uid, gid, path, mode);
   LogResult(backend_fd, "OpenFile({})", path);
   if (!backend_fd)
-    return ConvertResult(backend_fd.Error());
+    return {this, ConvertResult(backend_fd.Error()), ticks};
 
   auto& handle = m_fd_map[fd] = {gid, uid, backend_fd->Release()};
   std::strncpy(handle.name.data(), path.c_str(), handle.name.size());
-  return fd;
+  return {this, static_cast<s64>(fd), ticks};
 }
 
 std::optional<IPCReply> FSDevice::Close(u32 fd)

Source/Core/Core/IOS/FS/FileSystemProxy.h

@@ -24,12 +24,40 @@ constexpr FS::Fd INVALID_FD = 0xffffffff;
 class FSDevice : public Device
 {
 public:
+  class ScopedFd
+  {
+  public:
+    ScopedFd(FSDevice* fs, s64 fd, Ticks tick_tracker = {})
+        : m_fs{fs}, m_fd{fd}, m_tick_tracker{tick_tracker}
+    {
+    }
+
+    ~ScopedFd()
+    {
+      if (m_fd >= 0)
+        m_fs->Close(m_fd, m_tick_tracker);
+    }
+
+    ScopedFd(const ScopedFd&) = delete;
+    ScopedFd(ScopedFd&&) = delete;
+    ScopedFd& operator=(const ScopedFd&) = delete;
+    ScopedFd& operator=(ScopedFd&&) = delete;
+
+    s64 Get() const { return m_fd; }
+    s64 Release() { return std::exchange(m_fd, -1); }
+
+  private:
+    FSDevice* m_fs{};
+    s64 m_fd = -1;
+    Ticks m_tick_tracker{};
+  };
+
   FSDevice(Kernel& ios, const std::string& device_name);
 
   // These are the equivalent of the IPC command handlers so IPC overhead is included
   // in timing calculations.
-  s64 Open(FS::Uid uid, FS::Gid gid, const std::string& path, FS::Mode mode,
+  ScopedFd Open(FS::Uid uid, FS::Gid gid, const std::string& path, FS::Mode mode,
-           std::optional<u32> ipc_fd = {}, Ticks ticks = {});
+                std::optional<u32> ipc_fd = {}, Ticks ticks = {});
   s32 Close(u64 fd, Ticks ticks = {});
   s32 Read(u64 fd, u8* data, u32 size, std::optional<u32> ipc_buffer_addr = {}, Ticks ticks = {});
   s32 Write(u64 fd, const u8* data, u32 size, std::optional<u32> ipc_buffer_addr = {},

Source/Core/Core/IOS/IOS.cpp

@@ -346,16 +346,16 @@ u16 Kernel::GetGidForPPC() const
 static std::vector<u8> ReadBootContent(FSDevice* fs, const std::string& path, size_t max_size,
                                        Ticks ticks = {})
 {
-  const s64 fd = fs->Open(0, 0, path, FS::Mode::Read, {}, ticks);
+  const auto fd = fs->Open(0, 0, path, FS::Mode::Read, {}, ticks);
-  if (fd < 0)
+  if (fd.Get() < 0)
     return {};
 
-  const size_t file_size = fs->GetFileStatus(fd, ticks)->size;
+  const size_t file_size = fs->GetFileStatus(fd.Get(), ticks)->size;
   if (max_size != 0 && file_size > max_size)
     return {};
 
   std::vector<u8> buffer(file_size);
-  if (!fs->Read(fd, buffer.data(), buffer.size(), ticks))
+  if (!fs->Read(fd.Get(), buffer.data(), buffer.size(), ticks))
     return {};
   return buffer;
 }