From 74766fe6f9422f5fc5a53509bb0793980d226814 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Mon, 6 Oct 2025 22:25:05 -0500 Subject: [PATCH] get rid of the string copy and make the deep clone have a recursive limit --- dCommon/Lxfml.cpp | 22 ++++---- tests/dCommonTests/LxfmlTests.cpp | 84 +++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/dCommon/Lxfml.cpp b/dCommon/Lxfml.cpp index 6cc450ac..fd71be4c 100644 --- a/dCommon/Lxfml.cpp +++ b/dCommon/Lxfml.cpp @@ -38,11 +38,9 @@ Lxfml::Result Lxfml::NormalizePosition(const std::string_view data, const NiPoin return toReturn; } - // Ensure null-terminated string for tinyxml2::Parse - std::string nullTerminatedData(data); - tinyxml2::XMLDocument doc; - const auto err = doc.Parse(nullTerminatedData.c_str()); + // Use length-based parsing to avoid expensive string copy + const auto err = doc.Parse(data.data(), data.size()); if (err != tinyxml2::XML_SUCCESS) { return toReturn; } @@ -170,8 +168,9 @@ Lxfml::Result Lxfml::NormalizePosition(const std::string_view data, const NiPoin } // Deep-clone an XMLElement (attributes, text, and child elements) into a target document -static tinyxml2::XMLElement* CloneElementDeep(const tinyxml2::XMLElement* src, tinyxml2::XMLDocument& dstDoc) { - if (!src) return nullptr; +// with maximum depth protection to prevent infinite loops +static tinyxml2::XMLElement* CloneElementDeep(const tinyxml2::XMLElement* src, tinyxml2::XMLDocument& dstDoc, int maxDepth = 100) { + if (!src || maxDepth <= 0) return nullptr; auto* dst = dstDoc.NewElement(src->Name()); // copy attributes @@ -182,7 +181,9 @@ static tinyxml2::XMLElement* CloneElementDeep(const tinyxml2::XMLElement* src, t // copy children (elements and text) for (const tinyxml2::XMLNode* child = src->FirstChild(); child; child = child->NextSibling()) { if (const tinyxml2::XMLElement* childElem = child->ToElement()) { - dst->InsertEndChild(CloneElementDeep(childElem, dstDoc)); + // Recursively clone child elements with decremented depth + auto* clonedChild = CloneElementDeep(childElem, dstDoc, maxDepth - 1); + if (clonedChild) dst->InsertEndChild(clonedChild); } else if (const tinyxml2::XMLText* txt = child->ToText()) { auto* n = dstDoc.NewText(txt->Value()); dst->InsertEndChild(n); @@ -208,12 +209,9 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi return results; } - // Ensure null-terminated string for tinyxml2::Parse - // string_view::data() may not be null-terminated, causing undefined behavior - std::string nullTerminatedData(data); - tinyxml2::XMLDocument doc; - const auto err = doc.Parse(nullTerminatedData.c_str()); + // Use length-based parsing to avoid expensive string copy + const auto err = doc.Parse(data.data(), data.size()); if (err != tinyxml2::XML_SUCCESS) { return results; } diff --git a/tests/dCommonTests/LxfmlTests.cpp b/tests/dCommonTests/LxfmlTests.cpp index e366679e..68044031 100644 --- a/tests/dCommonTests/LxfmlTests.cpp +++ b/tests/dCommonTests/LxfmlTests.cpp @@ -296,3 +296,87 @@ TEST(LxfmlTests, MixedValidInvalidTransformsHandling) { }) << "Mixed valid/invalid transform processing should not cause fatal errors"; } } + +TEST(LxfmlTests, DeepCloneDepthProtection) { + // Test that deep cloning has protection against excessive nesting + // Create a deeply nested XML structure that would exceed reasonable limits + std::string deeplyNestedLxfml = R"( + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +)"; + + // The Split function should handle deeply nested structures without hanging + std::vector results; + EXPECT_NO_FATAL_FAILURE({ + results = Lxfml::Split(deeplyNestedLxfml); + }) << "Split should not hang or crash on deeply nested XML structures"; + + // Should still produce valid output despite depth limitations + EXPECT_GT(results.size(), 0) << "Should produce at least one result even with deep nesting"; + + if (results.size() > 0) { + // Verify the result is still valid XML + tinyxml2::XMLDocument doc; + auto parseResult = doc.Parse(results[0].lxfml.c_str()); + EXPECT_EQ(parseResult, tinyxml2::XML_SUCCESS) << "Result should still be valid XML"; + + if (parseResult == tinyxml2::XML_SUCCESS) { + auto* lxfml = doc.FirstChildElement("LXFML"); + EXPECT_NE(lxfml, nullptr) << "Result should have LXFML root element"; + + // Verify that bricks are still included despite group nesting issues + auto* bricks = lxfml->FirstChildElement("Bricks"); + EXPECT_NE(bricks, nullptr) << "Bricks element should be present"; + if (bricks) { + auto* brick = bricks->FirstChildElement("Brick"); + EXPECT_NE(brick, nullptr) << "At least one brick should be present"; + } + } + } +}