From 6eb9269741e35190ab554a27ab5aa54426e970dd Mon Sep 17 00:00:00 2001 From: Gary Miguel Date: Wed, 13 Dec 2023 04:15:37 -0800 Subject: [PATCH 1/2] 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); +} -- 2.47.0 From d1a55e9ca4e66d8ba2d799569de7a6b48bc15f23 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Wed, 13 Dec 2023 13:15:59 +0100 Subject: [PATCH 2/2] Remove use_texture_alpha compatibility code for nodeboxes & meshes (#13929) --- doc/lua_api.md | 4 ++-- src/nodedef.cpp | 58 ++----------------------------------------------- src/nodedef.h | 16 ++------------ 3 files changed, 6 insertions(+), 72 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index 54b366b55..c1cf1ea40 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -8837,8 +8837,8 @@ Used by `minetest.register_node`. -- depending on the alpha channel being below/above 50% in value -- * "blend": The alpha channel specifies how transparent a given pixel -- of the rendered node is - -- The default is "opaque" for drawtypes normal, liquid and flowingliquid; - -- "clip" otherwise. + -- The default is "opaque" for drawtypes normal, liquid and flowingliquid, + -- mesh and nodebox or "clip" otherwise. -- If set to a boolean value (deprecated): true either sets it to blend -- or clip, false sets it to clip or opaque mode depending on the drawtype. diff --git a/src/nodedef.cpp b/src/nodedef.cpp index cbfc77055..fa9198c5b 100644 --- a/src/nodedef.cpp +++ b/src/nodedef.cpp @@ -420,8 +420,6 @@ void ContentFeatures::reset() void ContentFeatures::setAlphaFromLegacy(u8 legacy_alpha) { - // No special handling for nodebox/mesh here as it doesn't make sense to - // throw warnings when the server is too old to support the "correct" way switch (drawtype) { case NDT_NORMAL: alpha = legacy_alpha == 255 ? ALPHAMODE_OPAQUE : ALPHAMODE_CLIP; @@ -648,6 +646,8 @@ void ContentFeatures::deSerialize(std::istream &is, u16 protocol_version) if (is.eof()) throw SerializationError(""); alpha = static_cast(tmp); + if (alpha == ALPHAMODE_LEGACY_COMPAT) + alpha = ALPHAMODE_OPAQUE; tmp = readU8(is); if (is.eof()) @@ -749,55 +749,6 @@ static void fillTileAttribs(ITextureSource *tsrc, TileLayer *layer, } } -bool ContentFeatures::textureAlphaCheck(ITextureSource *tsrc, const TileDef *tiles, int length) -{ - video::IVideoDriver *driver = RenderingEngine::get_video_driver(); - static thread_local bool long_warning_printed = false; - std::set seen; - - for (int i = 0; i < length; i++) { - if (seen.find(tiles[i].name) != seen.end()) - continue; - seen.insert(tiles[i].name); - - // Load the texture and see if there's any transparent pixels - video::ITexture *texture = tsrc->getTexture(tiles[i].name); - video::IImage *image = driver->createImage(texture, - core::position2d(0, 0), texture->getOriginalSize()); - if (!image) - continue; - core::dimension2d dim = image->getDimension(); - bool ok = true; - for (u16 x = 0; x < dim.Width; x++) { - for (u16 y = 0; y < dim.Height; y++) { - if (image->getPixel(x, y).getAlpha() < 255) { - ok = false; - goto break_loop; - } - } - } - -break_loop: - image->drop(); - if (ok) - continue; - warningstream << "Texture \"" << tiles[i].name << "\" of " - << name << " has transparency, assuming " - "use_texture_alpha = \"clip\"." << std::endl; - if (!long_warning_printed) { - warningstream << " This warning can be a false-positive if " - "unused pixels in the texture are transparent. However if " - "it is meant to be transparent, you *MUST* update the " - "nodedef and set use_texture_alpha = \"clip\"! This " - "compatibility code will be removed in a few releases." - << std::endl; - long_warning_printed = true; - } - return true; - } - return false; -} - bool isWorldAligned(AlignStyle style, WorldAlignMode mode, NodeDrawType drawtype) { if (style == ALIGN_STYLE_WORLD) @@ -841,11 +792,6 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc bool is_liquid = false; - if (alpha == ALPHAMODE_LEGACY_COMPAT) { - // Before working with the alpha mode, resolve any legacy kludges - alpha = textureAlphaCheck(tsrc, tdef, 6) ? ALPHAMODE_CLIP : ALPHAMODE_OPAQUE; - } - MaterialType material_type = alpha == ALPHAMODE_OPAQUE ? TILE_MATERIAL_OPAQUE : (alpha == ALPHAMODE_CLIP ? TILE_MATERIAL_BASIC : TILE_MATERIAL_ALPHA); diff --git a/src/nodedef.h b/src/nodedef.h index b1ec1e5ee..730a6cfc9 100644 --- a/src/nodedef.h +++ b/src/nodedef.h @@ -258,7 +258,7 @@ enum AlphaMode : u8 { ALPHAMODE_BLEND, ALPHAMODE_CLIP, ALPHAMODE_OPAQUE, - ALPHAMODE_LEGACY_COMPAT, /* means either opaque or clip */ + ALPHAMODE_LEGACY_COMPAT, /* only sent by old servers, equals OPAQUE */ }; @@ -466,11 +466,9 @@ struct ContentFeatures case NDT_NORMAL: case NDT_LIQUID: case NDT_FLOWINGLIQUID: - alpha = ALPHAMODE_OPAQUE; - break; case NDT_NODEBOX: case NDT_MESH: - alpha = ALPHAMODE_LEGACY_COMPAT; // this should eventually be OPAQUE + alpha = ALPHAMODE_OPAQUE; break; default: alpha = ALPHAMODE_CLIP; @@ -529,16 +527,6 @@ struct ContentFeatures #endif private: -#ifndef SERVER - /* - * Checks if any tile texture has any transparent pixels. - * Prints a warning and returns true if that is the case, false otherwise. - * This is supposed to be used for use_texture_alpha backwards compatibility. - */ - bool textureAlphaCheck(ITextureSource *tsrc, const TileDef *tiles, - int length); -#endif - void setAlphaFromLegacy(u8 legacy_alpha); u8 getAlphaForLegacy() const; -- 2.47.0