From 7f055a509a10b2214e3e8c601881aea6f6080b97 Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Sun, 28 Apr 2024 17:17:08 -0400 Subject: [PATCH] BUG: Fix STLFileReader crash bug (#930) - Original bug was caused by a malformed fmt::format statement with a mismatch '}' character. - Added more error checking to ensure we can safely recover from common STL reading errors - Updated the documentation to reflect the vendor list that does not write conformant STL Files. Signed-off-by: Michael Jackson --- .../SimplnxCore/docs/ReadStlFileFilter.md | 70 ++++++++++-- .../Filters/Algorithms/ReadStlFile.cpp | 100 +++++++++++++++--- .../Filters/Algorithms/ReadStlFile.hpp | 3 +- .../SimplnxCore/Filters/ReadStlFileFilter.cpp | 2 +- .../src/SimplnxCore/utils/StlUtilities.hpp | 1 + src/Plugins/SimplnxCore/test/CMakeLists.txt | 1 + .../SimplnxCore/test/ReadStlFileTest.cpp | 90 +++++++++++++++- 7 files changed, 241 insertions(+), 26 deletions(-) diff --git a/src/Plugins/SimplnxCore/docs/ReadStlFileFilter.md b/src/Plugins/SimplnxCore/docs/ReadStlFileFilter.md index 2376ec91d4..6174f4f7b7 100644 --- a/src/Plugins/SimplnxCore/docs/ReadStlFileFilter.md +++ b/src/Plugins/SimplnxCore/docs/ReadStlFileFilter.md @@ -12,14 +12,72 @@ This **Filter** will read a binary STL File and create a **Triangle Geometry** UINT32 Number of triangles foreach triangle - REAL32[3] Normal vector - REAL32[3] Vertex 1 - REAL32[3] Vertex 2 - REAL32[3] Vertex 3 - UINT16 Attribute byte count + REAL32[3] Normal vector + REAL32[3] Vertex 1 + REAL32[3] Vertex 2 + REAL32[3] Vertex 3 + UINT16 Attribute byte count end -**It is very important that the "Attribute byte Count" is correct as DREAM.3D follows the specification strictly.** If you are writing an STL file be sure that the value for the "Attribute byte count" is *zero* (0). If you chose to encode additional data into a section after each triangle then be sure that the "Attribute byte count" is set correctly. DREAM.3D will obey the value located in the "Attribute byte count". +The filter will look for specific header information to try and determine the vendor of the STL file. Certain vendors do not write STL files that adhere to the file spec. + +## IMPORANT NOTES: + +**It is very important that the "Attribute byte Count" is correct as DREAM3D-NX follows the specification strictly.** If you are writing an STL file be sure that the value for the "Attribute byte count" is *zero* (0). If you chose to encode additional data into a section after each triangle then be sure that the "Attribute byte count" is set correctly. DREAM3D-NX will obey the value located in the "Attribute byte count". + +## Known Vendors who Write out of spec STL Files + +- Materialise Magics [https://www.materialise.com/en/industrial/software/magics-data-build-preparation](https://www.materialise.com/en/industrial/software/magics-data-build-preparation) + + The filter looks in the header for "COLOR=" and "MATERIAL=" strings in the header. + +- Creaform VXelements [https://www.creaform3d.com/en/metrology-solutions/3d-applications-software-platforms](https://www.creaform3d.com/en/metrology-solutions/3d-applications-software-platforms) + + The filter looks for "VXelements" in the header. + +## Code to convert + +If you find yourself in a situation where the STL File is non-conforming and is not made by one of the vendors above, this bit of Python +code can clean up the file. This makes the absolute assumption that the **ONLY** thing wrong with the STL file is that the trailing UINT16 value for +each triangle needs to be set to ZERO. + + import struct + + def modify_stl(input_file_path, output_file_path): + with open(input_file_path, 'rb') as input_file, open(output_file_path, 'wb') as output_file: + # Read and copy header + header = input_file.read(80) + output_file.write(header) + + # Read number of triangles + num_triangles = struct.unpack(' #include @@ -92,10 +93,37 @@ std::array CreateMinMaxCoords() -std::numeric_limits::max(), std::numeric_limits::max(), -std::numeric_limits::max()}; } +bool IsMagicsFile(const std::string& stlHeaderStr) +{ + // Look for the tell-tale signs that the file was written from Magics Materialise + // If the file was written by Magics as a "Color STL" file then the 2byte int + // values between each triangle will be NON Zero which will screw up the reading. + // These NON Zero value do NOT indicate a length but is some sort of color + // value encoded into the file. Instead of being normal like everyone else and + // using the STL spec they went off and did their own thing. + + static const std::string k_ColorHeader("COLOR="); + static const std::string k_MaterialHeader("MATERIAL="); + if(stlHeaderStr.find(k_ColorHeader) != std::string::npos && stlHeaderStr.find(k_MaterialHeader) != std::string::npos) + { + return true; + } + return false; +} + +bool IsVxElementsFile(const std::string& stlHeader) +{ + // Look for the tell-tale signs that the file was written from VxElements Creaform + // Creaform STL files do not honor the last 2 bytes of the 50 byte Triangle struct + // as specified in the STL Binary File specification. If we detect this, then we + // ignore the 2 bytes are anything meaningful. + return nx::core::StringUtilities::contains(stlHeader, "VXelements"); +} + } // End anonymous namespace ReadStlFile::ReadStlFile(DataStructure& data, fs::path stlFilePath, const DataPath& geometryPath, const DataPath& faceGroupPath, const DataPath& faceNormalsDataPath, bool scaleOutput, - float32 scaleFactor, const std::atomic_bool& shouldCancel) + float32 scaleFactor, const std::atomic_bool& shouldCancel, const IFilter::MessageHandler& mesgHandler) : m_DataStructure(data) , m_FilePath(std::move(stlFilePath)) , m_GeometryDataPath(geometryPath) @@ -104,6 +132,7 @@ ReadStlFile::ReadStlFile(DataStructure& data, fs::path stlFilePath, const DataPa , m_ScaleOutput(scaleOutput) , m_ScaleFactor(scaleFactor) , m_ShouldCancel(shouldCancel) +, m_MessageHandler(mesgHandler) { } @@ -113,6 +142,8 @@ Result<> ReadStlFile::operator()() { m_MinMaxCoords = ::CreateMinMaxCoords(); + std::error_code errorCode; + auto stlFileSize = std::filesystem::file_size(m_FilePath, errorCode); // Open File FILE* f = std::fopen(m_FilePath.string().c_str(), "rb"); @@ -138,13 +169,8 @@ Result<> ReadStlFile::operator()() // using the STL spec they went off and did their own thing. std::string stlHeaderStr(stlHeader.data(), nx::core::StlConstants::k_STL_HEADER_LENGTH); - bool magicsFile = false; - static const std::string k_ColorHeader("COLOR="); - static const std::string k_MaterialHeader("MATERIAL="); - if(stlHeaderStr.find(k_ColorHeader) != std::string::npos && stlHeaderStr.find(k_MaterialHeader) != std::string::npos) - { - magicsFile = true; - } + bool ignoreMetaSizeValue = (IsMagicsFile(stlHeaderStr) || IsVxElementsFile(stlHeaderStr) ? true : false); + // Read the number of triangles in the file. if(std::fread(&triCount, sizeof(int32_t), 1, f) != 1) { @@ -169,25 +195,69 @@ Result<> ReadStlFile::operator()() std::array fileVert = {0.0F}; uint16_t attr = 0; std::vector triangleAttributeBuffer(std::numeric_limits::max()); // Just allocate a buffer of max UINT16 elements + + fpos_t pos; + auto start = std::chrono::steady_clock::now(); + int32_t progInt = 0; + for(int32_t t = 0; t < triCount; ++t) { + progInt = static_cast(t) / static_cast(triCount) * 100.0f; + + auto now = std::chrono::steady_clock::now(); + // Only send updates every 1 second + if(std::chrono::duration_cast(now - start).count() > 1000) + { + std::string message = fmt::format("Reading {}% Complete", progInt); + m_MessageHandler(nx::core::IFilter::ProgressMessage{nx::core::IFilter::Message::Type::Info, message, progInt}); + start = std::chrono::steady_clock::now(); + } + if(m_ShouldCancel) + { + return {}; + } + // Get the current File Position + fgetpos(f, &pos); +#if defined(__APPLE__) || defined(_WIN32) + if(pos >= stlFileSize) +#else + if(pos.__pos >= stlFileSize) +#endif + { + std::string msg = fmt::format( + "Trying to read at file position {} >= file size {}.\n File Header: '{}'\n Header Triangle Count: {} Current Triangle: {}\n The STL File does not conform to the STL file specification.", +#if defined(__APPLE__) || defined(_WIN32) + pos, +#else + pos.__pos, +#endif + stlFileSize, stlHeaderStr, triCount, t); + return MakeErrorResult(nx::core::StlConstants::k_StlFileLengthError, msg); + } + + // Read the Vertices and Normal (12 total float32 = 48 Bytes) size_t objsRead = std::fread(fileVert.data(), sizeof(float), k_StlElementCount, f); // Read the Triangle if(k_StlElementCount != objsRead) { - std::string msg = fmt::format("Error reading Triangle '{}}'. Object Count was {} and should have been {}", t, objsRead, k_StlElementCount); + std::string msg = fmt::format("Error reading Triangle '{}'. Object Count was {} and should have been {}", t, objsRead, k_StlElementCount); return MakeErrorResult(nx::core::StlConstants::k_TriangleParseError, msg); } - + // Read the Uint16 value that is supposed to represent the number of bytes following that are file/vendor specific meta data + // Lots of writers/vendors do NOT set this properly which can cause problems. objsRead = std::fread(&attr, sizeof(uint16_t), 1, f); // Read the Triangle Attribute Data length if(objsRead != 1) { - std::string msg = fmt::format("Error reading Number of attributes for triangle '{}'. Object Count was {} and should have been 1", t, objsRead); + std::string msg = fmt::format("Error reading Number of attributes for triangle '{}'. uint16 count was {} and should have been 1", t, objsRead); return MakeErrorResult(nx::core::StlConstants::k_AttributeParseError, msg); } - if(attr > 0 && !magicsFile) + // If we are trying to follow along the STL Spec, skip the stated bytes unless + // we detected known Vendors that do not write proper STL Files. + if(attr > 0 && !ignoreMetaSizeValue) { std::ignore = std::fseek(f, static_cast(attr), SEEK_CUR); // Skip past the Triangle Attribute data since we don't know how to read it anyways } + + // Determine the Min/Max Coordinates if(fileVert[3] < m_MinMaxCoords[0]) { m_MinMaxCoords[0] = fileVert[3]; @@ -260,6 +330,8 @@ Result<> ReadStlFile::operator()() { m_MinMaxCoords[5] = fileVert[11]; } + + // Write the data into the actual geometry faceNormals[3 * t + 0] = static_cast(fileVert[0]); faceNormals[3 * t + 1] = static_cast(fileVert[1]); faceNormals[3 * t + 2] = static_cast(fileVert[2]); @@ -275,10 +347,6 @@ Result<> ReadStlFile::operator()() triangles[t * 3] = 3 * t + 0; triangles[t * 3 + 1] = 3 * t + 1; triangles[t * 3 + 2] = 3 * t + 2; - if(m_ShouldCancel) - { - return {}; - } } return eliminate_duplicate_nodes(); diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadStlFile.hpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadStlFile.hpp index 7292336de5..fbcd6a8556 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadStlFile.hpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ReadStlFile.hpp @@ -23,7 +23,7 @@ class SIMPLNXCORE_EXPORT ReadStlFile { public: ReadStlFile(DataStructure& data, fs::path stlFilePath, const DataPath& geometryPath, const DataPath& faceGroupPath, const DataPath& faceNormalsDataPath, bool scaleOutput, float32 scaleFactor, - const std::atomic_bool& shouldCancel); + const std::atomic_bool& shouldCancel, const IFilter::MessageHandler& mesgHandler); ~ReadStlFile() noexcept; ReadStlFile(const ReadStlFile&) = delete; @@ -57,5 +57,6 @@ class SIMPLNXCORE_EXPORT ReadStlFile const bool m_ScaleOutput = false; const float m_ScaleFactor = 1.0F; const std::atomic_bool& m_ShouldCancel; + const IFilter::MessageHandler& m_MessageHandler; }; } // namespace nx::core diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ReadStlFileFilter.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ReadStlFileFilter.cpp index dec90dc579..40785d6635 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ReadStlFileFilter.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ReadStlFileFilter.cpp @@ -182,7 +182,7 @@ Result<> ReadStlFileFilter::executeImpl(DataStructure& data, const Arguments& fi auto scaleFactor = filterArgs.value(k_ScaleFactor); // The actual STL File Reading is placed in a separate class `ReadStlFile` - Result<> result = ReadStlFile(data, pStlFilePathValue, pTriangleGeometryPath, pFaceDataGroupPath, pFaceNormalsPath, scaleOutput, scaleFactor, shouldCancel)(); + Result<> result = ReadStlFile(data, pStlFilePathValue, pTriangleGeometryPath, pFaceDataGroupPath, pFaceNormalsPath, scaleOutput, scaleFactor, shouldCancel, messageHandler)(); return result; } diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/StlUtilities.hpp b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/StlUtilities.hpp index 9333358a4b..137ef22b6e 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/StlUtilities.hpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/StlUtilities.hpp @@ -19,6 +19,7 @@ inline constexpr int32_t k_StlHeaderParseError = -1104; inline constexpr int32_t k_TriangleCountParseError = -1105; inline constexpr int32_t k_TriangleParseError = -1106; inline constexpr int32_t k_AttributeParseError = -1107; +inline constexpr int32_t k_StlFileLengthError = -1108; enum class StlFileType : int { diff --git a/src/Plugins/SimplnxCore/test/CMakeLists.txt b/src/Plugins/SimplnxCore/test/CMakeLists.txt index f9d887112e..75b7e7050d 100644 --- a/src/Plugins/SimplnxCore/test/CMakeLists.txt +++ b/src/Plugins/SimplnxCore/test/CMakeLists.txt @@ -230,6 +230,7 @@ if(EXISTS "${DREAM3D_DATA_DIR}" AND SIMPLNX_DOWNLOAD_TEST_FILES) download_test_data(DREAM3D_DATA_DIR ${DREAM3D_DATA_DIR} ARCHIVE_NAME remove_flagged_triangles_test.tar.gz SHA512 cd5c6f3ea16a6d09e00e0c0bd0f941b27dca8a0beaeabb7262a2a6adaad83c829187c5d1aa433718123b628eaa839f016604c1134ced9f870723594b2df4be99) download_test_data(DREAM3D_DATA_DIR ${DREAM3D_DATA_DIR} ARCHIVE_NAME generate_color_table_test.tar.gz SHA512 b5683c758964eb723267400b14047f8adb0d5365ee9ca93d1a6940e9b6ad198cd4739c1ca799eb787b7706e668dbc16ab8243642034cdba5b71d64c27e682d3f) download_test_data(DREAM3D_DATA_DIR ${DREAM3D_DATA_DIR} ARCHIVE_NAME read_vtk_structured_points_test.tar.gz SHA512 e7a07a4e3901204c2562754cd71e0fdba1a46de2a5135bad2b6d66b40eefd0e63bed4dbe0ccd6ccadafb708ef63e20635d080aa3a35c172c4ced6986e0f75d5c) + download_test_data(DREAM3D_DATA_DIR ${DREAM3D_DATA_DIR} ARCHIVE_NAME ReadSTLFileTest.tar.gz SHA512 975587206625ffa183160308934e767347de55a34a16272cf5c121114efa286b3c6939e3c6a397e8728fdefe1771bc024bd4c9b409afdff0b76f2f56fcb9eb69) endif() diff --git a/src/Plugins/SimplnxCore/test/ReadStlFileTest.cpp b/src/Plugins/SimplnxCore/test/ReadStlFileTest.cpp index e6a5d4227b..c7af3236e8 100644 --- a/src/Plugins/SimplnxCore/test/ReadStlFileTest.cpp +++ b/src/Plugins/SimplnxCore/test/ReadStlFileTest.cpp @@ -16,8 +16,10 @@ namespace fs = std::filesystem; using namespace nx::core; using namespace nx::core::Constants; -TEST_CASE("SimplnxCore::ReadStlFileFilter", "[SimplnxCore][ReadStlFileFilter]") +TEST_CASE("SimplnxCore::ReadStlFileFilter:Valid_File", "[SimplnxCore][ReadStlFileFilter]") { + const nx::core::UnitTest::TestFileSentinel testDataSentinel(nx::core::unit_test::k_CMakeExecutable, nx::core::unit_test::k_TestFilesDir, "ReadSTLFileTest.tar.gz", "ReadSTLFileTest"); + // Instantiate the filter, a DataStructure object and an Arguments Object DataStructure dataStructure; Arguments args; @@ -25,7 +27,7 @@ TEST_CASE("SimplnxCore::ReadStlFileFilter", "[SimplnxCore][ReadStlFileFilter]") DataPath triangleGeomDataPath({"[Triangle Geometry]"}); - std::string inputFile = fmt::format("{}/ASTMD638_specimen.stl", unit_test::k_ComplexTestDataSourceDir.view()); + std::string inputFile = fmt::format("{}/ReadSTLFileTest/ASTMD638_specimen.stl", unit_test::k_TestFilesDir); // Create default Parameters for the filter. args.insertOrAssign(ReadStlFileFilter::k_StlFilePath_Key, std::make_any(fs::path(inputFile))); @@ -48,3 +50,87 @@ TEST_CASE("SimplnxCore::ReadStlFileFilter", "[SimplnxCore][ReadStlFileFilter]") WriteTestDataStructure(dataStructure, fs::path(fmt::format("{}/StlFileReaderTest.dream3d", unit_test::k_BinaryTestOutputDir))); #endif } + +TEST_CASE("SimplnxCore::ReadStlFileFilter:STLParseError", "[SimplnxCore][ReadStlFileFilter]") +{ + const nx::core::UnitTest::TestFileSentinel testDataSentinel(nx::core::unit_test::k_CMakeExecutable, nx::core::unit_test::k_TestFilesDir, "ReadSTLFileTest.tar.gz", "ReadSTLFileTest"); + + // Instantiate the filter, a DataStructure object and an Arguments Object + DataStructure dataStructure; + Arguments args; + ReadStlFileFilter filter; + + DataPath triangleGeomDataPath({"[Triangle Geometry]"}); + + std::string inputFile = fmt::format("{}/ReadSTLFileTest/stl_test_wrong_num_triangles.stl", unit_test::k_TestFilesDir); + + // Create default Parameters for the filter. + args.insertOrAssign(ReadStlFileFilter::k_StlFilePath_Key, std::make_any(fs::path(inputFile))); + args.insertOrAssign(ReadStlFileFilter::k_CreatedTriangleGeometryPath_Key, std::make_any(triangleGeomDataPath)); + + // Preflight the filter and check result + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + + // Execute the filter and check the result + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_INVALID(executeResult.result); + + REQUIRE(executeResult.result.errors().front().code == -1108); +} + +TEST_CASE("SimplnxCore::ReadStlFileFilter:TriangleParseError", "[SimplnxCore][ReadStlFileFilter]") +{ + const nx::core::UnitTest::TestFileSentinel testDataSentinel(nx::core::unit_test::k_CMakeExecutable, nx::core::unit_test::k_TestFilesDir, "ReadSTLFileTest.tar.gz", "ReadSTLFileTest"); + + // Instantiate the filter, a DataStructure object and an Arguments Object + DataStructure dataStructure; + Arguments args; + ReadStlFileFilter filter; + + DataPath triangleGeomDataPath({"[Triangle Geometry]"}); + + std::string inputFile = fmt::format("{}/ReadSTLFileTest/stl_test_2_TriangleParseError.stl", unit_test::k_TestFilesDir); + + // Create default Parameters for the filter. + args.insertOrAssign(ReadStlFileFilter::k_StlFilePath_Key, std::make_any(fs::path(inputFile))); + args.insertOrAssign(ReadStlFileFilter::k_CreatedTriangleGeometryPath_Key, std::make_any(triangleGeomDataPath)); + + // Preflight the filter and check result + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + + // Execute the filter and check the result + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_INVALID(executeResult.result); + + REQUIRE(executeResult.result.errors().front().code == -1106); +} + +TEST_CASE("SimplnxCore::ReadStlFileFilter:AttributeParseError", "[SimplnxCore][ReadStlFileFilter]") +{ + const nx::core::UnitTest::TestFileSentinel testDataSentinel(nx::core::unit_test::k_CMakeExecutable, nx::core::unit_test::k_TestFilesDir, "ReadSTLFileTest.tar.gz", "ReadSTLFileTest"); + + // Instantiate the filter, a DataStructure object and an Arguments Object + DataStructure dataStructure; + Arguments args; + ReadStlFileFilter filter; + + DataPath triangleGeomDataPath({"[Triangle Geometry]"}); + + std::string inputFile = fmt::format("{}/ReadSTLFileTest/stl_test_2_AttributeParseError.stl", unit_test::k_TestFilesDir); + + // Create default Parameters for the filter. + args.insertOrAssign(ReadStlFileFilter::k_StlFilePath_Key, std::make_any(fs::path(inputFile))); + args.insertOrAssign(ReadStlFileFilter::k_CreatedTriangleGeometryPath_Key, std::make_any(triangleGeomDataPath)); + + // Preflight the filter and check result + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + + // Execute the filter and check the result + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_INVALID(executeResult.result); + + REQUIRE(executeResult.result.errors().front().code == -1107); +}