From 87eded4bce2b71ff38672048b44a305a92e9967e Mon Sep 17 00:00:00 2001 From: Stenzek Date: Tue, 3 Sep 2024 20:43:10 +1000 Subject: [PATCH] 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 | 79 +++++++++++++++++++++++++++------- src/common/file_system.h | 4 +- src/core/game_database.cpp | 2 +- src/core/memory_card_image.cpp | 2 +- src/core/system.cpp | 4 +- 5 files changed, 71 insertions(+), 20 deletions(-) diff --git a/src/common/file_system.cpp b/src/common/file_system.cpp index b63a2d643..b02259543 100644 --- a/src/common/file_system.cpp +++ b/src/common/file_system.cpp @@ -1176,23 +1176,49 @@ void FileSystem::AtomicRenamedFileDeleter::operator()(std::FILE* fp) return; Error error; + + // final filename empty => discarded. + 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(errno); - ERROR_LOG("Failed to close temporary file '{}', discarding.", Path::GetFileName(m_temp_filename)); + Error::SetErrno(error, "fclose() failed: ", errno); m_final_filename.clear(); } - // final filename empty => discarded. - if (m_final_filename.empty()) + // Should not have been discarded. + if (!m_final_filename.empty()) { - if (!DeleteFile(m_temp_filename.c_str(), &error)) - ERROR_LOG("Failed to delete temporary file '{}': {}", Path::GetFileName(m_temp_filename), error.GetDescription()); + 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_LOG("Failed to rename temporary file '{}': {}", Path::GetFileName(m_temp_filename), error.GetDescription()); + Error::SetStringView(error, "File has already been discarded."); + 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, - Error* error /*= nullptr*/) +FileSystem::AtomicRenamedFile FileSystem::CreateAtomicRenamedFile(std::string filename, 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); -#elif defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__) - mkstemp(name_buf.get()); + const errno_t err = _mktemp_s(name_buf.get(), name_buf_size); + if (err == 0) + 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) diff --git a/src/common/file_system.h b/src/common/file_system.h index 7052af698..1d46d2897 100644 --- a/src/common/file_system.h +++ b/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; -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. diff --git a/src/core/game_database.cpp b/src/core/game_database.cpp index df88fc1fa..4b745c7d7 100644 --- a/src/core/game_database.cpp +++ b/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()); diff --git a/src/core/memory_card_image.cpp b/src/core/memory_card_image.cpp index 6bbf60121..39653dc95 100644 --- a/src/core/memory_card_image.cpp +++ b/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; diff --git a/src/core/system.cpp b/src/core/system.cpp index 29dfb490f..8a3883839 100644 --- a/src/core/system.cpp +++ b/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 */)