FileSystem: Fix FD leak with atomic updated file on Linux

Also add the ability to explicitly commit and check for errors.

src/common/file_system.cpp

@@ -1176,23 +1176,49 @@ void FileSystem::AtomicRenamedFileDeleter::operator()(std::FILE* fp)
     return;
 
   Error error;
-  if (std::fclose(fp) != 0)
-  {
-    error.SetErrno(errno);
-    ERROR_LOG("Failed to close temporary file '{}', discarding.", Path::GetFileName(m_temp_filename));
-    m_final_filename.clear();
-  }
 
   // final filename empty => discarded.
-  if (m_final_filename.empty())
+  if (!m_final_filename.empty())
   {
+    if (!commit(fp, &error))
+    {
+      ERROR_LOG("Failed to commit temporary file '{}', discarding. Error was {}.", Path::GetFileName(m_temp_filename),
+                error.GetDescription());
+    }
+
+    return;
+  }
+
+  // we're discarding the file, don't care if it fails.
+  std::fclose(fp);
+
   if (!DeleteFile(m_temp_filename.c_str(), &error))
     ERROR_LOG("Failed to delete temporary file '{}': {}", Path::GetFileName(m_temp_filename), error.GetDescription());
 }
+
+bool FileSystem::AtomicRenamedFileDeleter::commit(std::FILE* fp, Error* error)
+{
+  if (!fp) [[unlikely]]
+  {
+    Error::SetStringView(error, "File pointer is null.");
+    return false;
+  }
+
+  if (std::fclose(fp) != 0)
+  {
+    Error::SetErrno(error, "fclose() failed: ", errno);
+    m_final_filename.clear();
+  }
+
+  // Should not have been discarded.
+  if (!m_final_filename.empty())
+  {
+    return RenamePath(m_temp_filename.c_str(), m_final_filename.c_str(), error);
+  }
   else
   {
-    if (!RenamePath(m_temp_filename.c_str(), m_final_filename.c_str(), &error))
+    Error::SetStringView(error, "File has already been discarded.");
-      ERROR_LOG("Failed to rename temporary file '{}': {}", Path::GetFileName(m_temp_filename), error.GetDescription());
+    return DeleteFile(m_temp_filename.c_str(), error);
   }
 }
 
@@ -1201,8 +1227,7 @@ void FileSystem::AtomicRenamedFileDeleter::discard()
   m_final_filename = {};
 }
 
-FileSystem::AtomicRenamedFile FileSystem::CreateAtomicRenamedFile(std::string filename, const char* mode,
+FileSystem::AtomicRenamedFile FileSystem::CreateAtomicRenamedFile(std::string filename, Error* error /*= nullptr*/)
-                                                                  Error* error /*= nullptr*/)
 {
   std::string temp_filename;
   std::FILE* fp = nullptr;
@@ -1216,14 +1241,29 @@ FileSystem::AtomicRenamedFile FileSystem::CreateAtomicRenamedFile(std::string fi
     StringUtil::Strlcpy(name_buf.get() + filename_length, ".XXXXXX", name_buf_size);
 
 #ifdef _WIN32
-    _mktemp_s(name_buf.get(), name_buf_size);
+    const errno_t err = _mktemp_s(name_buf.get(), name_buf_size);
-#elif defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__)
+    if (err == 0)
-    mkstemp(name_buf.get());
+      fp = OpenCFile(name_buf.get(), "w+b", error);
+    else
+      Error::SetErrno(error, "_mktemp_s() failed: ", err);
+
+#elif defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__) || defined(__FreeBSD__)
+    const int fd = mkstemp(name_buf.get());
+    if (fd >= 0)
+    {
+      fp = fdopen(fd, "w+b");
+      if (!fp)
+        Error::SetErrno(error, "fdopen() failed: ", errno);
+    }
+    else
+    {
+      Error::SetErrno(error, "mkstemp() failed: ", errno);
+    }
 #else
     mktemp(name_buf.get());
+    fp = OpenCFile(name_buf.get(), "w+b", error);
 #endif
 
-    fp = OpenCFile(name_buf.get(), mode, error);
     if (fp)
       temp_filename.assign(name_buf.get(), name_buf_size - 1);
     else
@@ -1236,7 +1276,7 @@ FileSystem::AtomicRenamedFile FileSystem::CreateAtomicRenamedFile(std::string fi
 bool FileSystem::WriteAtomicRenamedFile(std::string filename, const void* data, size_t data_length,
                                         Error* error /*= nullptr*/)
 {
-  AtomicRenamedFile fp = CreateAtomicRenamedFile(std::move(filename), "wb", error);
+  AtomicRenamedFile fp = CreateAtomicRenamedFile(std::move(filename), error);
   if (!fp)
     return false;
 
@@ -1255,6 +1295,15 @@ void FileSystem::DiscardAtomicRenamedFile(AtomicRenamedFile& file)
   file.get_deleter().discard();
 }
 
+bool FileSystem::CommitAtomicRenamedFile(AtomicRenamedFile& file, Error* error)
+{
+  if (file.get_deleter().commit(file.release(), error))
+    return true;
+
+  Error::AddPrefix(error, "Failed to commit file: ");
+  return false;
+}
+
 #endif
 
 FileSystem::ManagedCFilePtr FileSystem::OpenManagedCFile(const char* filename, const char* mode, Error* error)

src/common/file_system.h

@@ -147,6 +147,7 @@ public:
   ~AtomicRenamedFileDeleter();
 
   void operator()(std::FILE* fp);
+  bool commit(std::FILE* fp, Error* error); // closes file
   void discard();
 
 private:
@@ -154,8 +155,9 @@ private:
   std::string m_final_filename;
 };
 using AtomicRenamedFile = std::unique_ptr<std::FILE, AtomicRenamedFileDeleter>;
-AtomicRenamedFile CreateAtomicRenamedFile(std::string filename, const char* mode, Error* error = nullptr);
+AtomicRenamedFile CreateAtomicRenamedFile(std::string filename, Error* error = nullptr);
 bool WriteAtomicRenamedFile(std::string filename, const void* data, size_t data_length, Error* error = nullptr);
+bool CommitAtomicRenamedFile(AtomicRenamedFile& file, Error* error);
 void DiscardAtomicRenamedFile(AtomicRenamedFile& file);
 
 /// Abstracts a POSIX file lock.

src/core/game_database.cpp

@@ -1002,7 +1002,7 @@ bool GameDatabase::SaveToCache()
   const u64 gamedb_ts = Host::GetResourceFileTimestamp("gamedb.yaml", false).value_or(0);
 
   Error error;
-  FileSystem::AtomicRenamedFile file = FileSystem::CreateAtomicRenamedFile(GetCacheFile(), "wb", &error);
+  FileSystem::AtomicRenamedFile file = FileSystem::CreateAtomicRenamedFile(GetCacheFile(), &error);
   if (!file)
   {
     ERROR_LOG("Failed to open cache file for writing: {}", error.GetDescription());

src/core/memory_card_image.cpp

@@ -646,7 +646,7 @@ bool MemoryCardImage::ExportSave(DataArray* data, const FileInfo& fi, const char
   if (!ReadFile(*data, fi, &file_data, error))
     return false;
 
-  auto fp = FileSystem::CreateAtomicRenamedFile(filename, "wb", error);
+  auto fp = FileSystem::CreateAtomicRenamedFile(filename, error);
   if (!fp)
     return false;
 

src/core/system.cpp

@@ -2910,7 +2910,7 @@ bool System::SaveState(const char* path, Error* error, bool backup_existing_save
     }
   }
 
-  auto fp = FileSystem::CreateAtomicRenamedFile(path, "wb", error);
+  auto fp = FileSystem::CreateAtomicRenamedFile(path, error);
   if (!fp)
   {
     Error::AddPrefixFmt(error, "Cannot open '{}': ", Path::GetFileName(path));
@@ -2930,7 +2930,7 @@ bool System::SaveState(const char* path, Error* error, bool backup_existing_save
                           5.0f);
 
   VERBOSE_LOG("Saving state took {:.2f} msec", save_timer.GetTimeMilliseconds());
-  return true;
+  return FileSystem::CommitAtomicRenamedFile(fp, error);
 }
 
 bool System::SaveStateToBuffer(SaveStateBuffer* buffer, Error* error, u32 screenshot_size /* = 256 */)