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.
This commit is contained in:
Gary Miguel 2023-12-13 04:15:37 -08:00 committed by GitHub
parent a98200bb4c
commit 6eb9269741
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 49 additions and 18 deletions

@ -758,14 +758,32 @@ bool safeWriteToFile(const std::string &path, const std::string &content)
std::string tmp_file = path + ".~mt"; std::string tmp_file = path + ".~mt";
// Write to a tmp file // Write to a tmp file
std::ofstream os(tmp_file.c_str(), std::ios::binary); bool tmp_success = false;
if (!os.good())
#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; 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 << content;
os.flush(); os.flush();
os.close(); os.close();
if (os.fail()) { tmp_success = !os.fail();
// Remove the temporary file because writing it failed and it's useless. #endif
if (!tmp_success) {
remove(tmp_file.c_str()); remove(tmp_file.c_str());
return false; 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 // 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. // 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 // We retry up to 5 times, with a 1ms sleep between, before we consider the whole operation failed
int number_attempts = 0; for (int attempt = 0; attempt < 5; attempt++) {
while (number_attempts < 5) {
rename_success = MoveFileEx(tmp_file.c_str(), path.c_str(), rename_success = MoveFileEx(tmp_file.c_str(), path.c_str(),
MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH); MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH);
if (rename_success) if (rename_success)
break; break;
sleep_ms(1); sleep_ms(1);
++number_attempts;
} }
#else #else
// On POSIX compliant systems rename() is specified to be able to swap the // On POSIX compliant systems rename() is specified to be able to swap the

@ -9,7 +9,7 @@ set (UNITTEST_SRCS
${CMAKE_CURRENT_SOURCE_DIR}/test_compression.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_compression.cpp
${CMAKE_CURRENT_SOURCE_DIR}/test_connection.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_connection.cpp
${CMAKE_CURRENT_SOURCE_DIR}/test_craft.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_inventory.cpp
${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp
${CMAKE_CURRENT_SOURCE_DIR}/test_lua.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_lua.cpp

@ -26,10 +26,11 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#include "nodedef.h" #include "nodedef.h"
#include "noise.h" #include "noise.h"
class TestFilePath : public TestBase { class TestFileSys : public TestBase
{
public: public:
TestFilePath() { TestManager::registerTestModule(this); } TestFileSys() { TestManager::registerTestModule(this); }
const char *getName() { return "TestFilePath"; } const char *getName() { return "TestFileSys"; }
void runTests(IGameDef *gamedef); void runTests(IGameDef *gamedef);
@ -38,17 +39,19 @@ public:
void testRemoveLastPathComponent(); void testRemoveLastPathComponent();
void testRemoveLastPathComponentWithTrailingDelimiter(); void testRemoveLastPathComponentWithTrailingDelimiter();
void testRemoveRelativePathComponent(); 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(testIsDirDelimiter);
TEST(testPathStartsWith); TEST(testPathStartsWith);
TEST(testRemoveLastPathComponent); TEST(testRemoveLastPathComponent);
TEST(testRemoveLastPathComponentWithTrailingDelimiter); TEST(testRemoveLastPathComponentWithTrailingDelimiter);
TEST(testRemoveRelativePathComponent); 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('/') == true);
UASSERT(fs::IsDirDelimiter('A') == false); UASSERT(fs::IsDirDelimiter('A') == false);
@ -87,7 +90,7 @@ void TestFilePath::testIsDirDelimiter()
} }
void TestFilePath::testPathStartsWith() void TestFileSys::testPathStartsWith()
{ {
const int numpaths = 12; const int numpaths = 12;
std::string paths[numpaths] = { std::string paths[numpaths] = {
@ -163,7 +166,7 @@ void TestFilePath::testPathStartsWith()
} }
void TestFilePath::testRemoveLastPathComponent() void TestFileSys::testRemoveLastPathComponent()
{ {
std::string path, result, removed; std::string path, result, removed;
@ -200,7 +203,7 @@ void TestFilePath::testRemoveLastPathComponent()
} }
void TestFilePath::testRemoveLastPathComponentWithTrailingDelimiter() void TestFileSys::testRemoveLastPathComponentWithTrailingDelimiter()
{ {
std::string path, result, removed; std::string path, result, removed;
@ -236,7 +239,7 @@ void TestFilePath::testRemoveLastPathComponentWithTrailingDelimiter()
} }
void TestFilePath::testRemoveRelativePathComponent() void TestFileSys::testRemoveRelativePathComponent()
{ {
std::string path, result; std::string path, result;
@ -262,3 +265,15 @@ void TestFilePath::testRemoveRelativePathComponent()
result = fs::RemoveRelativePathComponents(path); result = fs::RemoveRelativePathComponents(path);
UASSERT(result == p("/a/e")); 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);
}