From 6eb9269741e35190ab554a27ab5aa54426e970dd Mon Sep 17 00:00:00 2001 From: Gary Miguel Date: Wed, 13 Dec 2023 04:15:37 -0800 Subject: [PATCH] Try to fix safeWriteToFile producing empty files on Windows (#14085) Use win32 APIs to write the temporary file before copying to the final destination. Because we've observed the final file being empty, we suspect that std::ostream::flush is not flushing. Also add a test for it. --- src/filesys.cpp | 30 ++++++++++++---- src/unittest/CMakeLists.txt | 2 +- .../{test_filepath.cpp => test_filesys.cpp} | 35 +++++++++++++------ 3 files changed, 49 insertions(+), 18 deletions(-) rename src/unittest/{test_filepath.cpp => test_filesys.cpp} (89%) diff --git a/src/filesys.cpp b/src/filesys.cpp index d91199594..416f09da8 100644 --- a/src/filesys.cpp +++ b/src/filesys.cpp @@ -758,14 +758,32 @@ bool safeWriteToFile(const std::string &path, const std::string &content) std::string tmp_file = path + ".~mt"; // Write to a tmp file - std::ofstream os(tmp_file.c_str(), std::ios::binary); - if (!os.good()) + bool tmp_success = false; + +#ifdef _WIN32 + // We've observed behavior suggesting that the MSVC implementation of std::ofstream::flush doesn't + // actually flush, so we use win32 APIs. + HANDLE tmp_handle = CreateFile( + tmp_file.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); + if (tmp_handle == INVALID_HANDLE_VALUE) { return false; + } + DWORD bytes_written; + tmp_success = (WriteFile(tmp_handle, content.c_str(), content.size(), &bytes_written, nullptr) && + FlushFileBuffers(tmp_handle)); + CloseHandle(tmp_handle); +#else + std::ofstream os(tmp_file.c_str(), std::ios::binary); + if (!os.good()) { + return false; + } os << content; os.flush(); os.close(); - if (os.fail()) { - // Remove the temporary file because writing it failed and it's useless. + tmp_success = !os.fail(); +#endif + + if (!tmp_success) { remove(tmp_file.c_str()); return false; } @@ -777,14 +795,12 @@ bool safeWriteToFile(const std::string &path, const std::string &content) // When creating the file, it can cause Windows Search indexer, virus scanners and other apps // to query the file. This can make the move file call below fail. // We retry up to 5 times, with a 1ms sleep between, before we consider the whole operation failed - int number_attempts = 0; - while (number_attempts < 5) { + for (int attempt = 0; attempt < 5; attempt++) { rename_success = MoveFileEx(tmp_file.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH); if (rename_success) break; sleep_ms(1); - ++number_attempts; } #else // On POSIX compliant systems rename() is specified to be able to swap the diff --git a/src/unittest/CMakeLists.txt b/src/unittest/CMakeLists.txt index c43a7dbd3..fd47358a2 100644 --- a/src/unittest/CMakeLists.txt +++ b/src/unittest/CMakeLists.txt @@ -9,7 +9,7 @@ set (UNITTEST_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/test_compression.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_connection.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_craft.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/test_filepath.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test_filesys.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_inventory.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_lua.cpp diff --git a/src/unittest/test_filepath.cpp b/src/unittest/test_filesys.cpp similarity index 89% rename from src/unittest/test_filepath.cpp rename to src/unittest/test_filesys.cpp index f1a79062d..8b63d083f 100644 --- a/src/unittest/test_filepath.cpp +++ b/src/unittest/test_filesys.cpp @@ -26,10 +26,11 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "nodedef.h" #include "noise.h" -class TestFilePath : public TestBase { +class TestFileSys : public TestBase +{ public: - TestFilePath() { TestManager::registerTestModule(this); } - const char *getName() { return "TestFilePath"; } + TestFileSys() { TestManager::registerTestModule(this); } + const char *getName() { return "TestFileSys"; } void runTests(IGameDef *gamedef); @@ -38,17 +39,19 @@ public: void testRemoveLastPathComponent(); void testRemoveLastPathComponentWithTrailingDelimiter(); void testRemoveRelativePathComponent(); + void testSafeWriteToFile(); }; -static TestFilePath g_test_instance; +static TestFileSys g_test_instance; -void TestFilePath::runTests(IGameDef *gamedef) +void TestFileSys::runTests(IGameDef *gamedef) { TEST(testIsDirDelimiter); TEST(testPathStartsWith); TEST(testRemoveLastPathComponent); TEST(testRemoveLastPathComponentWithTrailingDelimiter); TEST(testRemoveRelativePathComponent); + TEST(testSafeWriteToFile); } //////////////////////////////////////////////////////////////////////////////// @@ -74,7 +77,7 @@ std::string p(std::string path) } -void TestFilePath::testIsDirDelimiter() +void TestFileSys::testIsDirDelimiter() { UASSERT(fs::IsDirDelimiter('/') == true); UASSERT(fs::IsDirDelimiter('A') == false); @@ -87,7 +90,7 @@ void TestFilePath::testIsDirDelimiter() } -void TestFilePath::testPathStartsWith() +void TestFileSys::testPathStartsWith() { const int numpaths = 12; std::string paths[numpaths] = { @@ -163,7 +166,7 @@ void TestFilePath::testPathStartsWith() } -void TestFilePath::testRemoveLastPathComponent() +void TestFileSys::testRemoveLastPathComponent() { std::string path, result, removed; @@ -200,7 +203,7 @@ void TestFilePath::testRemoveLastPathComponent() } -void TestFilePath::testRemoveLastPathComponentWithTrailingDelimiter() +void TestFileSys::testRemoveLastPathComponentWithTrailingDelimiter() { std::string path, result, removed; @@ -236,7 +239,7 @@ void TestFilePath::testRemoveLastPathComponentWithTrailingDelimiter() } -void TestFilePath::testRemoveRelativePathComponent() +void TestFileSys::testRemoveRelativePathComponent() { std::string path, result; @@ -262,3 +265,15 @@ void TestFilePath::testRemoveRelativePathComponent() result = fs::RemoveRelativePathComponents(path); UASSERT(result == p("/a/e")); } + + +void TestFileSys::testSafeWriteToFile() +{ + const std::string dest_path = fs::TempPath() + DIR_DELIM + "testSafeWriteToFile.txt"; + const std::string test_data("hello\0world", 11); + fs::safeWriteToFile(dest_path, test_data); + UASSERT(fs::PathExists(dest_path)); + std::string contents_actual; + UASSERT(fs::ReadFile(dest_path, contents_actual)); + UASSERT(contents_actual == test_data); +}