From a438edd976ceb343ac65559b596d68a1a00111ed Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:17:09 -0400 Subject: [PATCH 1/9] llxml: modernize LLXMLNode construction, tidy escapeXML Replace the triplicated constructor init lists with default member initializers in the header; the constructors now only set what differs per overload. This also fixes a latent -Wreorder in the copy constructor (mParser/mParent were initialized out of declaration order). Drop the unused include (nothing references LLDir), and give escapeXML a reserve() plus a range-for. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llxml/llxmlnode.cpp | 70 ++++----------------------------------- indra/llxml/llxmlnode.h | 26 +++++++-------- 2 files changed, 20 insertions(+), 76 deletions(-) diff --git a/indra/llxml/llxmlnode.cpp b/indra/llxml/llxmlnode.cpp index 215a1af8a0..091cd79851 100644 --- a/indra/llxml/llxmlnode.cpp +++ b/indra/llxml/llxmlnode.cpp @@ -46,75 +46,25 @@ #include "llsd.h" #include "llstring.h" #include "lluuid.h" -#include "lldir.h" // static bool LLXMLNode::sStripEscapedStrings = true; bool LLXMLNode::sStripWhitespaceValues = false; -LLXMLNode::LLXMLNode() : - mID(""), - mParser(NULL), - mIsAttribute(false), - mVersionMajor(0), - mVersionMinor(0), - mLength(0), - mPrecision(64), - mType(TYPE_CONTAINER), - mEncoding(ENCODING_DEFAULT), - mLineNumber(-1), - mParent(NULL), - mChildren(NULL), - mAttributes(), - mPrev(NULL), - mNext(NULL), - mName(NULL), - mValue(""), - mDefault(NULL) +// Most members are initialized from default member initializers in the header. +LLXMLNode::LLXMLNode() { } LLXMLNode::LLXMLNode(const char* name, bool is_attribute) : - mID(""), - mParser(NULL), mIsAttribute(is_attribute), - mVersionMajor(0), - mVersionMinor(0), - mLength(0), - mPrecision(64), - mType(TYPE_CONTAINER), - mEncoding(ENCODING_DEFAULT), - mLineNumber(-1), - mParent(NULL), - mChildren(NULL), - mAttributes(), - mPrev(NULL), - mNext(NULL), - mValue(""), - mDefault(NULL) -{ - mName = gStringTable.addStringEntry(name); + mName(gStringTable.addStringEntry(name)) +{ } LLXMLNode::LLXMLNode(LLStringTableEntry* name, bool is_attribute) : - mID(""), - mParser(NULL), mIsAttribute(is_attribute), - mVersionMajor(0), - mVersionMinor(0), - mLength(0), - mPrecision(64), - mType(TYPE_CONTAINER), - mEncoding(ENCODING_DEFAULT), - mLineNumber(-1), - mParent(NULL), - mChildren(NULL), - mAttributes(), - mPrev(NULL), - mNext(NULL), - mName(name), - mValue(""), - mDefault(NULL) + mName(name) { } @@ -129,12 +79,6 @@ LLXMLNode::LLXMLNode(const LLXMLNode& rhs) : mType(rhs.mType), mEncoding(rhs.mEncoding), mLineNumber(0), - mParser(NULL), - mParent(NULL), - mChildren(NULL), - mAttributes(), - mPrev(NULL), - mNext(NULL), mName(rhs.mName), mValue(rhs.mValue), mDefault(rhs.mDefault) @@ -2510,9 +2454,9 @@ void LLXMLNode::setDoubleValue(U32 length, const F64 *array, Encoding encoding, std::string LLXMLNode::escapeXML(const std::string& xml) { std::string out; - for (std::string::size_type i = 0; i < xml.size(); ++i) + out.reserve(xml.size()); + for (char c : xml) { - char c = xml[i]; switch(c) { case '"': out.append("""); break; diff --git a/indra/llxml/llxmlnode.h b/indra/llxml/llxmlnode.h index 57b7dc5811..615a5bfc57 100644 --- a/indra/llxml/llxmlnode.h +++ b/indra/llxml/llxmlnode.h @@ -295,18 +295,18 @@ class LLXMLNode : public LLThreadSafeRefCount public: std::string mID; // The ID attribute of this node - XML_Parser *mParser; // Temporary pointer while loading - - bool mIsAttribute; // Flag is only used for output formatting - U32 mVersionMajor; // Version of this tag to use - U32 mVersionMinor; - U32 mLength; // If the length is nonzero, then only return arrays of this length - U32 mPrecision; // The number of BITS per array item - ValueType mType; // The value type - Encoding mEncoding; // The value encoding - S32 mLineNumber; // line number in source file, if applicable - - LLXMLNode* mParent; // The parent node + XML_Parser *mParser{ nullptr }; // Temporary pointer while loading + + bool mIsAttribute{ false }; // Flag is only used for output formatting + U32 mVersionMajor{ 0 }; // Version of this tag to use + U32 mVersionMinor{ 0 }; + U32 mLength{ 0 }; // If the length is nonzero, then only return arrays of this length + U32 mPrecision{ 64 }; // The number of BITS per array item + ValueType mType{ TYPE_CONTAINER }; // The value type + Encoding mEncoding{ ENCODING_DEFAULT }; // The value encoding + S32 mLineNumber{ -1 }; // line number in source file, if applicable + + LLXMLNode* mParent{ nullptr }; // The parent node LLXMLChildrenPtr mChildren; // The child nodes LLXMLAttribList mAttributes; // The attribute nodes LLXMLNodePtr mPrev; // Double-linked list previous node @@ -316,7 +316,7 @@ class LLXMLNode : public LLThreadSafeRefCount static bool sStripWhitespaceValues; protected: - LLStringTableEntry *mName; // The name of this node + LLStringTableEntry *mName{ nullptr }; // The name of this node // The value of this node (use getters/setters only) // Values are not XML-escaped in memory From 7e40561209ab076e728f42715624c90af8ce25c5 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:18:06 -0400 Subject: [PATCH 2/9] llxml: cut allocations and O(n^2) value building in the parser The character-data callback (XMLData) read the node's entire accumulated value out via getValue() and wrote it back via setValue() on every chunk, making multi-chunk text (anything over expat's buffer, entity refs, etc.) quadratic. Add appendValue(string_view) and a setValue(string&&) overload and append in place instead - linear in the text length. StartXMLNode now inspects attribute names as string_view (the expat atts array is already NUL-terminated), moves each attribute value into its node once, and drops the dead per-attribute dedup lookup (expat already rejects duplicate attribute names) plus an unused local. EndXMLNode no longer copies the value to test it for whitespace. setBoolValue built its string with llformat("%s ...", val.c_str(), ...), re-formatting the whole accumulator each element; switch to append. All output is byte-for-byte identical. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llxml/llxmlnode.cpp | 126 +++++++++++++++++++------------------- indra/llxml/llxmlnode.h | 4 ++ 2 files changed, 68 insertions(+), 62 deletions(-) diff --git a/indra/llxml/llxmlnode.cpp b/indra/llxml/llxmlnode.cpp index 091cd79851..64e3b2eb37 100644 --- a/indra/llxml/llxmlnode.cpp +++ b/indra/llxml/llxmlnode.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include "llxmlnode.h" @@ -329,7 +330,6 @@ void XMLCALL StartXMLNode(void *userData, LLXMLNodePtr new_node = new_node_ptr; new_node->mID.clear(); - LLXMLNodePtr ptr_new_node = new_node; // Set the parent-child relationship with the current active node LLXMLNode* parent = (LLXMLNode *)userData; @@ -341,29 +341,31 @@ void XMLCALL StartXMLNode(void *userData, } new_node_ptr->mParser = parent->mParser; - new_node_ptr->setLineNumber(XML_GetCurrentLineNumber(*new_node_ptr->mParser)); + const S32 line_number = XML_GetCurrentLineNumber(*new_node_ptr->mParser); + new_node_ptr->setLineNumber(line_number); // Set the current active node to the new node XML_Parser *parser = parent->mParser; XML_SetUserData(*parser, (void *)new_node_ptr); - // Parse attributes + // Parse attributes. atts is name/value pairs; both are NUL-terminated, so + // the names can be inspected as string_views without allocating. U32 pos = 0; while (atts[pos] != NULL) { - std::string attr_name = atts[pos]; - std::string attr_value = atts[pos+1]; + const std::string_view attr_name = atts[pos]; + const std::string_view attr_value = atts[pos + 1]; // Special cases if ('i' == attr_name[0] && "id" == attr_name) { - new_node->mID = attr_value; + new_node->mID = atts[pos + 1]; } else if ('v' == attr_name[0] && "version" == attr_name) { U32 version_major = 0; U32 version_minor = 0; - if (sscanf(attr_value.c_str(), "%d.%d", &version_major, &version_minor) > 0) + if (sscanf(atts[pos + 1], "%d.%d", &version_major, &version_minor) > 0) { new_node->mVersionMajor = version_major; new_node->mVersionMinor = version_minor; @@ -372,7 +374,7 @@ void XMLCALL StartXMLNode(void *userData, else if (('s' == attr_name[0] && "size" == attr_name) || ('l' == attr_name[0] && "length" == attr_name)) { U32 length; - if (sscanf(attr_value.c_str(), "%d", &length) > 0) + if (sscanf(atts[pos + 1], "%d", &length) > 0) { new_node->mLength = length; } @@ -380,7 +382,7 @@ void XMLCALL StartXMLNode(void *userData, else if ('p' == attr_name[0] && "precision" == attr_name) { U32 precision; - if (sscanf(attr_value.c_str(), "%d", &precision) > 0) + if (sscanf(atts[pos + 1], "%d", &precision) > 0) { new_node->mPrecision = precision; } @@ -428,14 +430,11 @@ void XMLCALL StartXMLNode(void *userData, }*/ } - // only one attribute child per description - LLXMLNodePtr attr_node; - if (!new_node->getAttribute(attr_name.c_str(), attr_node, false)) - { - attr_node = new LLXMLNode(attr_name.c_str(), true); - attr_node->setLineNumber(XML_GetCurrentLineNumber(*new_node_ptr->mParser)); - } - attr_node->setValue(attr_value); + // Attribute names are unique within an element (expat rejects + // duplicates), so the matching child cannot already exist - create it. + LLXMLNodePtr attr_node = new LLXMLNode(atts[pos], true); + attr_node->setLineNumber(line_number); + attr_node->setValue(std::string(attr_value)); new_node->addChild(attr_node); pos += 2; @@ -457,21 +456,11 @@ void XMLCALL EndXMLNode(void *userData, // SJB: total hack: if (LLXMLNode::sStripWhitespaceValues) { - std::string value = node->getValue(); - bool is_empty = true; - for (std::string::size_type s = 0; s < value.length(); s++) + // If the value is empty or all whitespace, clear it (this also flips the + // type from TYPE_CONTAINER to TYPE_UNKNOWN, as setValue does). + if (node->getValue().find_first_not_of(" \t\n") == std::string::npos) { - char c = value[s]; - if (c != ' ' && c != '\t' && c != '\n') - { - is_empty = false; - break; - } - } - if (is_empty) - { - value.clear(); - node->setValue(value); + node->setValue(LLStringUtil::null); } } } @@ -481,37 +470,35 @@ void XMLCALL XMLData(void *userData, int len) { LLXMLNode* current_node = (LLXMLNode *)userData; - std::string value = current_node->getValue(); - if (LLXMLNode::sStripEscapedStrings) + if (LLXMLNode::sStripEscapedStrings && len > 0 && s[0] == '\"' && s[len - 1] == '\"') { - if (s[0] == '\"' && s[len-1] == '\"') + // Special-case: Escaped string. Unescape into a scratch buffer and + // append it directly to the node value. Appending (rather than copying + // the accumulated value out and back) keeps multi-chunk text linear + // instead of O(n^2). + std::string unescaped_string; + unescaped_string.reserve(len > 2 ? (size_t)(len - 2) : 0); + for (S32 pos=1; possetValue(value); - return; } + current_node->appendValue(unescaped_string); + return; } - value.append(std::string(s, len)); - current_node->setValue(value); + current_node->appendValue(std::string_view(s, len)); } @@ -2134,15 +2121,12 @@ void LLXMLNode::setBoolValue(U32 length, const bool *array) { if (pos > 0) { - new_value = llformat("%s %s", new_value.c_str(), array[pos]?"true":"false"); - } - else - { - new_value = array[pos]?"true":"false"; + new_value.push_back(' '); } + new_value.append(array[pos] ? "true" : "false"); } - mValue = new_value; + mValue = std::move(new_value); mEncoding = ENCODING_DEFAULT; mLength = length; mType = TYPE_BOOLEAN; @@ -2538,6 +2522,24 @@ void LLXMLNode::setValue(const std::string& value) mValue = value; } +void LLXMLNode::setValue(std::string&& value) +{ + if (TYPE_CONTAINER == mType) + { + mType = TYPE_UNKNOWN; + } + mValue = std::move(value); +} + +void LLXMLNode::appendValue(std::string_view value) +{ + if (TYPE_CONTAINER == mType) + { + mType = TYPE_UNKNOWN; + } + mValue.append(value); +} + void LLXMLNode::setDefault(LLXMLNode *default_node) { mDefault = default_node; diff --git a/indra/llxml/llxmlnode.h b/indra/llxml/llxmlnode.h index 615a5bfc57..3a03f35378 100644 --- a/indra/llxml/llxmlnode.h +++ b/indra/llxml/llxmlnode.h @@ -254,6 +254,10 @@ class LLXMLNode : public LLThreadSafeRefCount void setUUIDValue(U32 length, const LLUUID *array); void setNodeRefValue(U32 length, const LLXMLNode **array); void setValue(const std::string& value); + void setValue(std::string&& value); + // Append to the node value without copying the existing contents out and + // back (used by the parser's character-data callback). + void appendValue(std::string_view value); void setName(const std::string& name); void setName(LLStringTableEntry* name); From 51afe878901540bacc8f2bbec5069bcf966dcc92 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:18:26 -0400 Subject: [PATCH 3/9] llxml: avoid per-chunk allocation in LLXmlTreeParser::characterData characterData built a std::string for every character-data chunk even when not dumping, then appended that copy to the node contents. Take a (const char*, len) overload of appendContents and append the chunk directly; only materialize a string in the (rarely taken) dump branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llxml/llxmltree.cpp | 14 ++++++++------ indra/llxml/llxmltree.h | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/indra/llxml/llxmltree.cpp b/indra/llxml/llxmltree.cpp index d66544d0f8..d0bc78d460 100644 --- a/indra/llxml/llxmltree.cpp +++ b/indra/llxml/llxmltree.cpp @@ -181,9 +181,9 @@ LLXmlTreeNode* LLXmlTreeNode::getNextNamedChild() return (mChildMapIter++)->second; } -void LLXmlTreeNode::appendContents(const std::string& str) +void LLXmlTreeNode::appendContents(const char* str, std::string::size_type len) { - mContents.append( str ); + mContents.append( str, len ); } void LLXmlTreeNode::addChild(LLXmlTreeNode* child) @@ -612,16 +612,18 @@ void LLXmlTreeParser::endElement(const char* name) void LLXmlTreeParser::characterData(const char *s, int len) { - std::string str; - if (s) str = std::string(s, len); + if (!s || len <= 0) + { + return; + } if( mDump ) { - LL_INFOS() << tabs() << "CharacterData " << str << LL_ENDL; + LL_INFOS() << tabs() << "CharacterData " << std::string(s, len) << LL_ENDL; } if (mKeepContents) { - mCurrent->appendContents( str ); + mCurrent->appendContents( s, len ); } } diff --git a/indra/llxml/llxmltree.h b/indra/llxml/llxmltree.h index 120d7d3e56..4165278362 100644 --- a/indra/llxml/llxmltree.h +++ b/indra/llxml/llxmltree.h @@ -164,7 +164,7 @@ class LLXmlTreeNode private: void addAttribute( const std::string& name, const std::string& value ); - void appendContents( const std::string& str ); + void appendContents( const char* str, std::string::size_type len ); void addChild( LLXmlTreeNode* child ); void dump( const std::string& prefix ); From 681628027ea29e36adb20baa3f8a5d305d09a305 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:18:45 -0400 Subject: [PATCH 4/9] llxml: add LLXMLNode unit tests Stand up the previously-empty llxml unit-test target with TUT coverage for the parse path that the preceding commits touched: attribute/value round-trip, escaped-string unescaping, multi-chunk text accumulation (via parseStream's 1KB chunks), typed float arrays, bool serialization, escapeXML, and deepCopy. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llxml/CMakeLists.txt | 6 +- indra/llxml/tests/llxmlnode_test.cpp | 156 +++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 indra/llxml/tests/llxmlnode_test.cpp diff --git a/indra/llxml/CMakeLists.txt b/indra/llxml/CMakeLists.txt index 2090e829ad..effccaf2f1 100644 --- a/indra/llxml/CMakeLists.txt +++ b/indra/llxml/CMakeLists.txt @@ -36,7 +36,11 @@ target_precompile_headers(llxml REUSE_FROM llprecompiled) if (BUILD_TESTING) # unit tests SET(llxml_TEST_SOURCE_FILES - # none yet! + llxmlnode.cpp + ) + set_source_files_properties(llxmlnode.cpp + PROPERTIES + LL_TEST_ADDITIONAL_LIBRARIES "ll::expat" ) LL_ADD_PROJECT_UNIT_TESTS(llxml "${llxml_TEST_SOURCE_FILES}") diff --git a/indra/llxml/tests/llxmlnode_test.cpp b/indra/llxml/tests/llxmlnode_test.cpp new file mode 100644 index 0000000000..dfd3f09478 --- /dev/null +++ b/indra/llxml/tests/llxmlnode_test.cpp @@ -0,0 +1,156 @@ +/** + * @file llxmlnode_test.cpp + * @brief LLXMLNode unit tests + * + * $LicenseInfo:firstyear=2025&license=viewerlgpl$ + * Second Life Viewer Source Code + * Copyright (C) 2025, Linden Research, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA + * $/LicenseInfo$ + */ + +#include "linden_common.h" +#include "../test/lltut.h" + +#include "../llxmlnode.h" + +#include +#include + +namespace tut +{ + struct llxmlnode_data + { + }; + typedef test_group llxmlnode_test; + typedef llxmlnode_test::object llxmlnode_object; + tut::llxmlnode_test llxmlnode_testcase("llxmlnode"); + + // Element name, id, attributes (string + int) and text value round-trip. + template<> template<> + void llxmlnode_object::test<1>() + { + const std::string xml = + "body text"; + LLXMLNodePtr node; + ensure("parseBuffer succeeds", + LLXMLNode::parseBuffer(xml.data(), xml.size(), node, nullptr)); + ensure("node non-null", node.notNull()); + ensure("node name", node->hasName("thing")); + ensure_equals("id attribute", node->getID(), std::string("abc")); + ensure_equals("text value", node->getValue(), std::string("body text")); + + std::string name; + ensure("name attr present", node->getAttributeString("name", name)); + ensure_equals("name attr value", name, std::string("hello world")); + + S32 count = 0; + ensure("count attr present", node->getAttributeS32("count", count)); + ensure_equals("count attr value", count, 42); + } + + // Escaped-string handling: a value wholly wrapped in double quotes is + // unescaped (\" -> ", \\ -> \) since sStripEscapedStrings defaults to true. + template<> template<> + void llxmlnode_object::test<2>() + { + const std::string xml = "\"abc\\\"def\""; // raw: "abc\"def" + LLXMLNodePtr node; + ensure("parse", LLXMLNode::parseBuffer(xml.data(), xml.size(), node, nullptr)); + ensure_equals("unescaped value", node->getValue(), std::string("abc\"def")); + } + + // parseStream feeds expat in 1KB chunks, so a long body arrives across many + // character-data callbacks. Guards value accumulation (the append path). + template<> template<> + void llxmlnode_object::test<3>() + { + const size_t N = 5000; + std::string xml = ""; + xml.append(N, 'x'); + xml += ""; + std::istringstream stream(xml); + LLXMLNodePtr node; + ensure("parseStream", LLXMLNode::parseStream(stream, node, nullptr)); + ensure_equals("accumulated length", node->getValue().size(), N); + ensure("all content preserved", node->getValue() == std::string(N, 'x')); + } + + // Typed float array parsing via a child element. + template<> template<> + void llxmlnode_object::test<4>() + { + const std::string xml = + "1.5 -2.25 3.0"; + LLXMLNodePtr node; + ensure("parse", LLXMLNode::parseBuffer(xml.data(), xml.size(), node, nullptr)); + + LLXMLNodePtr pos; + ensure("getChild pos", node->getChild("pos", pos)); + F32 v[3] = { 0.f, 0.f, 0.f }; + ensure_equals("3 floats parsed", (S32)pos->getFloatValue(3, v), 3); + ensure("v0", std::fabs(v[0] - 1.5f) < 1e-5f); + ensure("v1", std::fabs(v[1] + 2.25f) < 1e-5f); + ensure("v2", std::fabs(v[2] - 3.0f) < 1e-5f); + } + + // Boolean array serialization (guards the whitespace-joined append path). + template<> template<> + void llxmlnode_object::test<5>() + { + LLXMLNodePtr node = new LLXMLNode("flags", false); + const bool vals[3] = { true, false, true }; + node->setBoolValue(3, vals); + ensure_equals("bool serialization", node->getValue(), + std::string("true false true")); + + bool out[3] = { false, false, false }; + ensure_equals("read back count", (S32)node->getBoolValue(3, out), 3); + ensure("b0", out[0] == true); + ensure("b1", out[1] == false); + ensure("b2", out[2] == true); + } + + // XML escaping of the five special characters. + template<> template<> + void llxmlnode_object::test<6>() + { + ensure_equals("escapeXML", + LLXMLNode::escapeXML("a&\"'c"), + std::string("a<b>&"'c")); + } + + // deepCopy uses the copy constructor; verify fields survive the copy. + template<> template<> + void llxmlnode_object::test<7>() + { + const std::string xml = "text"; + LLXMLNodePtr node; + ensure("parse", LLXMLNode::parseBuffer(xml.data(), xml.size(), node, nullptr)); + + LLXMLNodePtr copy = node->deepCopy(); + ensure("copy non-null", copy.notNull()); + ensure("copy name", copy->hasName("a")); + ensure_equals("copy id", copy->getID(), std::string("i1")); + ensure_equals("copy value", copy->getValue(), std::string("text")); + + std::string k; + ensure("copy attr present", copy->getAttributeString("k", k)); + ensure_equals("copy attr value", k, std::string("v")); + } +} From 4296a19e3c81e6ff126f8840318a8f6599ef35b6 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:31:12 -0400 Subject: [PATCH 5/9] llcommon: drop dead string-table hash_map paths, modernize LLStringTable STRING_TABLE_HASH_MAP is only ever defined under _MSC_VER 1300-1399 (VS2002/2003), and its branches use std::hash_multimap / __gnu_cxx:: hash_multimap, neither of which exists in modern stdlib. Remove the dead branches from the header and from check/add/removeStringEntry and the destructor, leaving the live list-bucket implementation. Also drop the #if 0 alternate hash in hash_my_string, value-initialize the bucket array, inline the now-dead ret_val locals, and switch NULL -> nullptr. LLStringTableEntry owns a raw char* with a user-declared destructor but had an implicit copy ctor (shallow copy -> double free). It is never copied (always heap-allocated and held by pointer), so delete the copy ctor/assignment to make the single-owner invariant explicit. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llcommon/llstringtable.cpp | 142 ++++--------------------------- indra/llcommon/llstringtable.h | 23 ++--- 2 files changed, 22 insertions(+), 143 deletions(-) diff --git a/indra/llcommon/llstringtable.cpp b/indra/llcommon/llstringtable.cpp index d28e1e2219..f3d2aa7e05 100644 --- a/indra/llcommon/llstringtable.cpp +++ b/indra/llcommon/llstringtable.cpp @@ -52,11 +52,10 @@ LLStringTableEntry::~LLStringTableEntry() LLStringTable::LLStringTable(int tablesize) : mUniqueEntries(0) { - S32 i; if (!tablesize) tablesize = 4096; // some arbitrary default // Make sure tablesize is power of 2 - for (i = 31; i>0; i--) + for (S32 i = 31; i>0; i--) { if (tablesize & (1< P = mStringHash.equal_range(hash_value); - string_hash_t::iterator lower = P.first; - string_hash_t::iterator upper = P.second; -#endif - for (string_hash_t::iterator iter = lower; iter != upper; iter++) - { - entry = iter->second; - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) - { - return entry; - } - } -#else - string_list_t *strlist = mStringList[hash_value]; + U32 hash_value = hash_my_string(str, mMaxEntries); + string_list_t* strlist = mStringList[hash_value]; if (strlist) { for (LLStringTableEntry* entry : *strlist) { - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) + if (!strncmp(entry->mString, str, MAX_STRINGS_LENGTH)) { return entry; } } } -#endif } - return NULL; + return nullptr; } char* LLStringTable::addString(const std::string& str) @@ -222,42 +177,14 @@ LLStringTableEntry* LLStringTable::addStringEntry(const char *str) { if (str) { - char *ret_val = NULL; - U32 hash_value = hash_my_string(str, mMaxEntries); -#if STRING_TABLE_HASH_MAP - LLStringTableEntry *entry; -#if 1 // Microsoft - string_hash_t::iterator lower = mStringHash.lower_bound(hash_value); - string_hash_t::iterator upper = mStringHash.upper_bound(hash_value); -#else // stlport - std::pair P = mStringHash.equal_range(hash_value); - string_hash_t::iterator lower = P.first; - string_hash_t::iterator upper = P.second; -#endif - for (string_hash_t::iterator iter = lower; iter != upper; iter++) - { - entry = iter->second; - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) - { - entry->incCount(); - return entry; - } - } - - // not found, so add! - LLStringTableEntry* newentry = new LLStringTableEntry(str); - ret_val = newentry->mString; - mStringHash.insert(string_hash_t::value_type(hash_value, newentry)); -#else - string_list_t *strlist = mStringList[hash_value]; + U32 hash_value = hash_my_string(str, mMaxEntries); + string_list_t* strlist = mStringList[hash_value]; if (strlist) { for (LLStringTableEntry* entry : *strlist) { - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) + if (!strncmp(entry->mString, str, MAX_STRINGS_LENGTH)) { entry->incCount(); return entry; @@ -272,15 +199,13 @@ LLStringTableEntry* LLStringTable::addStringEntry(const char *str) // not found, so add! LLStringTableEntry *newentry = new LLStringTableEntry(str); - //ret_val = newentry->mString; strlist->push_front(newentry); -#endif mUniqueEntries++; return newentry; } else { - return NULL; + return nullptr; } } @@ -288,48 +213,14 @@ void LLStringTable::removeString(const char *str) { if (str) { - char *ret_val; - U32 hash_value = hash_my_string(str, mMaxEntries); -#if STRING_TABLE_HASH_MAP - { - LLStringTableEntry *entry; -#if 1 // Microsoft - string_hash_t::iterator lower = mStringHash.lower_bound(hash_value); - string_hash_t::iterator upper = mStringHash.upper_bound(hash_value); -#else // stlport - std::pair P = mStringHash.equal_range(hash_value); - string_hash_t::iterator lower = P.first; - string_hash_t::iterator upper = P.second; -#endif - for (string_hash_t::iterator iter = lower; iter != upper; iter++) - { - entry = iter->second; - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) - { - if (!entry->decCount()) - { - mUniqueEntries--; - if (mUniqueEntries < 0) - { - LL_ERRS() << "LLStringTable:removeString trying to remove too many strings!" << LL_ENDL; - } - delete iter->second; - mStringHash.erase(iter); - } - return; - } - } - } -#else - string_list_t *strlist = mStringList[hash_value]; + U32 hash_value = hash_my_string(str, mMaxEntries); + string_list_t* strlist = mStringList[hash_value]; if (strlist) { for (LLStringTableEntry* entry : *strlist) { - ret_val = entry->mString; - if (!strncmp(ret_val, str, MAX_STRINGS_LENGTH)) + if (!strncmp(entry->mString, str, MAX_STRINGS_LENGTH)) { if (!entry->decCount()) { @@ -345,7 +236,6 @@ void LLStringTable::removeString(const char *str) } } } -#endif } } diff --git a/indra/llcommon/llstringtable.h b/indra/llcommon/llstringtable.h index 97f95369e5..95841b6011 100644 --- a/indra/llcommon/llstringtable.h +++ b/indra/llcommon/llstringtable.h @@ -34,14 +34,6 @@ #include #include -#if LL_WINDOWS -# if (_MSC_VER >= 1300 && _MSC_VER < 1400) -# define STRING_TABLE_HASH_MAP 1 -# endif -#else -//# define STRING_TABLE_HASH_MAP 1 -#endif - const U32 MAX_STRINGS_LENGTH = 256; class LL_COMMON_API LLStringTableEntry @@ -50,6 +42,12 @@ class LL_COMMON_API LLStringTableEntry LLStringTableEntry(const char *str); ~LLStringTableEntry(); + // Owns a raw char buffer and is always heap-allocated and referenced by + // pointer (never copied), so delete copy to make that single-owner + // invariant explicit and rule out an accidental double-free. + LLStringTableEntry(const LLStringTableEntry&) = delete; + LLStringTableEntry& operator=(const LLStringTableEntry&) = delete; + void incCount() { mCount++; } bool decCount() { return --mCount != 0; } @@ -77,18 +75,9 @@ class LL_COMMON_API LLStringTable S32 mMaxEntries; S32 mUniqueEntries; -#if STRING_TABLE_HASH_MAP -#if LL_WINDOWS - typedef std::hash_multimap string_hash_t; -#else - typedef __gnu_cxx::hash_multimap string_hash_t; -#endif - string_hash_t mStringHash; -#else typedef std::list string_list_t; typedef string_list_t * string_list_ptr_t; string_list_ptr_t *mStringList; -#endif }; extern LL_COMMON_API LLStringTable gStringTable; From 010dcb072bcaee740cf250d8ced047ae79df06e8 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:31:25 -0400 Subject: [PATCH 6/9] llui: fix LLXUIParser defects and trim the read hot path readXUI() dereferenced node (node->getName()) before its isNull() check, and the warning branch further dereferenced a null/stale mCurReadNode; check for null first and bail out. ScopedFile's destructor called fclose() even when the file failed to open (fclose(NULL) is UB); guard it. readXUIImpl() called getSanitizedValue() up to twice per node; compute it once and reuse. Use find('.') instead of find(".") for the single-char scan, and emplace_back name-stack/output-stack entries instead of push_back(make_pair(...)). Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llui/llxuiparser.cpp | 52 ++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/indra/llui/llxuiparser.cpp b/indra/llui/llxuiparser.cpp index 2365d0fe23..a818a5fec1 100644 --- a/indra/llui/llxuiparser.cpp +++ b/indra/llui/llxuiparser.cpp @@ -678,20 +678,22 @@ const LLXMLNodePtr DUMMY_NODE = new LLXMLNode(); void LLXUIParser::readXUI(LLXMLNodePtr node, LLInitParam::BaseBlock& block, const std::string& filename, bool silent) { LL_RECORD_BLOCK_TIME(FTM_PARSE_XUI); + + if (node.isNull()) + { + // Can't use parserWarning() here: it dereferences mCurReadNode, which + // is null/stale before any node of this parse has been read. + LL_WARNS() << "Invalid (null) node passed to readXUI" << LL_ENDL; + return; + } + mNameStack.clear(); mRootNodeName = node->getName()->mString; mCurFileName = filename; mCurReadDepth = 0; setParseSilently(silent); - if (node.isNull()) - { - parserWarning("Invalid node"); - } - else - { - readXUIImpl(node, block); - } + readXUIImpl(node, block); } bool LLXUIParser::readXUIImpl(LLXMLNodePtr nodep, LLInitParam::BaseBlock& block) @@ -702,9 +704,13 @@ bool LLXUIParser::readXUIImpl(LLXMLNodePtr nodep, LLInitParam::BaseBlock& block) bool values_parsed = false; bool silent = mCurReadDepth > 0; + // getSanitizedValue() does real work; compute it once and reuse it for both + // the empty-node check and the "value" parameter below. + std::string text_contents = nodep->getSanitizedValue(); + if (nodep->getFirstChild().isNull() && nodep->mAttributes.empty() - && nodep->getSanitizedValue().empty()) + && text_contents.empty()) { // empty node, just parse as flag mCurReadNode = DUMMY_NODE; @@ -715,11 +721,10 @@ bool LLXUIParser::readXUIImpl(LLXMLNodePtr nodep, LLInitParam::BaseBlock& block) values_parsed |= readAttributes(nodep, block); // treat text contents of xml node as "value" parameter - std::string text_contents = nodep->getSanitizedValue(); if (!text_contents.empty()) { mCurReadNode = nodep; - mNameStack.push_back(std::make_pair(std::string("value"), true)); + mNameStack.emplace_back("value", true); // child nodes are not necessarily valid parameters (could be a child widget) // so don't complain once we've recursed if (!block.submitValue(mNameStack, *this, true)) @@ -752,9 +757,9 @@ bool LLXUIParser::readXUIImpl(LLXMLNodePtr nodep, LLInitParam::BaseBlock& block) // and if not, treat as a child element of the current node // e.g. will interpret as "button.rect" // since there is no widget named "rect" - if (child_name.find(".") == std::string::npos) + if (child_name.find('.') == std::string::npos) { - mNameStack.push_back(std::make_pair(child_name, true)); + mNameStack.emplace_back(child_name, true); num_tokens_pushed++; } else @@ -790,7 +795,7 @@ bool LLXUIParser::readXUIImpl(LLXMLNodePtr nodep, LLInitParam::BaseBlock& block) // copy remaining tokens on to our running token list for(tokenizer::iterator token_to_push = name_token_it; token_to_push != name_tokens.end(); ++token_to_push) { - mNameStack.push_back(std::make_pair(*token_to_push, true)); + mNameStack.emplace_back(*token_to_push, true); num_tokens_pushed++; } } @@ -840,7 +845,7 @@ bool LLXUIParser::readAttributes(LLXMLNodePtr nodep, LLInitParam::BaseBlock& blo // copy remaining tokens on to our running token list for(tokenizer::iterator token_to_push = name_tokens.begin(); token_to_push != name_tokens.end(); ++token_to_push) { - mNameStack.push_back(std::make_pair(*token_to_push, true)); + mNameStack.emplace_back(*token_to_push, true); num_tokens_pushed++; } @@ -1330,8 +1335,11 @@ struct ScopedFile ~ScopedFile() { - fclose(mFile); - mFile = NULL; + if (mFile) + { + fclose(mFile); + mFile = NULL; + } } S32 getRemainingBytes() @@ -1388,7 +1396,7 @@ bool LLSimpleXUIParser::readXUI(const std::string& filename, LLInitParam::BaseBl XML_SetElementHandler( mParser, startElementHandler, endElementHandler); XML_SetCharacterDataHandler( mParser, characterDataHandler); - mOutputStack.push_back(std::make_pair(&block, 0)); + mOutputStack.emplace_back(&block, 0); mNameStack.clear(); mCurFileName = filename; mCurReadDepth = 0; @@ -1471,7 +1479,7 @@ void LLSimpleXUIParser::startElement(const char *name, const char **atts) LLInitParam::BaseBlock* blockp = mElementCB(*this, name); if (blockp) { - mOutputStack.push_back(std::make_pair(blockp, 0)); + mOutputStack.emplace_back(blockp, 0); } } @@ -1485,7 +1493,7 @@ void LLSimpleXUIParser::startElement(const char *name, const char **atts) } else { // compound attribute - if (child_name.find(".") == std::string::npos) + if (child_name.find('.') == std::string::npos) { mNameStack.emplace_back(child_name, true); num_tokens_pushed++; @@ -1514,7 +1522,7 @@ void LLSimpleXUIParser::startElement(const char *name, const char **atts) // copy remaining tokens on to our running token list for(tokenizer::iterator token_to_push = name_token_it; token_to_push != name_tokens.end(); ++token_to_push) { - mNameStack.push_back(std::make_pair(*token_to_push, true)); + mNameStack.emplace_back(*token_to_push, true); num_tokens_pushed++; } mScope.push_back(mNameStack.back().first); @@ -1578,7 +1586,7 @@ bool LLSimpleXUIParser::readAttributes(const char **atts) // copy remaining tokens on to our running token list for(tokenizer::iterator token_to_push = name_tokens.begin(); token_to_push != name_tokens.end(); ++token_to_push) { - mNameStack.push_back(std::make_pair(*token_to_push, true)); + mNameStack.emplace_back(*token_to_push, true); num_tokens_pushed++; } From 234eab44161acbd228baf679c148a962ca5e7429 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:32:11 -0400 Subject: [PATCH 7/9] llcommon: reimplement LLInitParam LazyValue over std::unique_ptr Replace the hand-rolled new/delete and manual rule-of-three with a std::unique_ptr member. Deep-copy value semantics and lazy heap allocation are preserved -- heap storage (rather than std::optional) keeps a Lazy<> param small when T is a large, usually-absent block -- and move ctor/assignment are added (the type was previously copy-only). Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llcommon/llinitparam.h | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h index 5464ad7e01..95b6130bd1 100644 --- a/indra/llcommon/llinitparam.h +++ b/indra/llcommon/llinitparam.h @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include #include @@ -670,39 +672,37 @@ namespace LLInitParam class BaseBlock* mCurrentBlockPtr; // pointer to block currently being constructed }; - // TODO: implement in terms of owned_ptr + // Optional, lazily-allocated, heap-stored T with deep-copy value semantics. + // Heap storage (rather than e.g. std::optional) keeps a Lazy<> param small + // even when T is a large block that is usually absent. template class LazyValue { public: LazyValue() = default; - ~LazyValue() { delete mPtr; } + LazyValue(const T& value) : mPtr(std::make_unique(value)) {} - LazyValue(const T& value) { mPtr = new T(value); } - - LazyValue(const LazyValue& other) : mPtr(nullptr) { *this = other; } + LazyValue(const LazyValue& other) { *this = other; } + LazyValue(LazyValue&& other) noexcept = default; LazyValue& operator=(const LazyValue& other) { if (!other.mPtr) { - delete mPtr; - mPtr = nullptr; + mPtr.reset(); + } + else if (!mPtr) + { + mPtr = std::make_unique(*other.mPtr); } else { - if (!mPtr) - { - mPtr = new T(*other.mPtr); - } - else - { - *mPtr = *(other.mPtr); - } + *mPtr = *other.mPtr; } return *this; } + LazyValue& operator=(LazyValue&& other) noexcept = default; bool operator==(const LazyValue& other) const { From 927c92ea96f77edd1c2aef62a2e8fddc1b5ea103 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 00:32:35 -0400 Subject: [PATCH 8/9] llcommon: LLInitParam defect fixes, copy reductions, and moves Defects: - Multiple::isValid() used strict < while the registered validate() uses <=, so a Multiple at exactly its min/max count (e.g. AtLeast<1> with a single element) reported not-provided. Make both bounds inclusive. - getPossibleValues() appended to a function-static vector with no guard, so the inspect path (XSD/RNG writers) grew it unboundedly with duplicate entries. Populate it once. Copy/alloc reductions: - getValueName()/calcValueName() return const std::string& instead of by value, removing string copies in the serialize loops. - serializeBlock()/inspectBlock() build the unnamed-handle set once for O(1) duplicate checks instead of an O(named*unnamed) rescan per param. - Non-block Multiple deserialize parses directly into a freshly-added element (popping on failure) instead of into a local that is then copied in, matching how block-Multiple already works. Moves: - emplace_back over push_back(make_pair(...)); mValues = value over std::copy + back_inserter; add() uses emplace_back; rvalue set(container_t&&) plus matching Multiple assignment/call operators. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llcommon/llinitparam.cpp | 50 +++++++-------- indra/llcommon/llinitparam.h | 107 ++++++++++++++++++++++----------- 2 files changed, 99 insertions(+), 58 deletions(-) diff --git a/indra/llcommon/llinitparam.cpp b/indra/llcommon/llinitparam.cpp index 9de2dd8594..73017f83c3 100644 --- a/indra/llcommon/llinitparam.cpp +++ b/indra/llcommon/llinitparam.cpp @@ -30,6 +30,8 @@ #include "llinitparam.h" #include "llformat.h" +#include + namespace LLInitParam { @@ -145,7 +147,7 @@ namespace LLInitParam if (param->mValidationFunc) { - mValidationList.push_back(std::make_pair(param->mParamHandle, param->mValidationFunc)); + mValidationList.emplace_back(param->mParamHandle, param->mValidationFunc); } } @@ -246,6 +248,16 @@ namespace LLInitParam } } + // Precompute the set of unnamed (implicit) param handles so the loop + // below can skip already-serialized params in O(1) instead of rescanning + // mUnnamedParams for every named param. + std::unordered_set unnamed_handles; + unnamed_handles.reserve(block_data.mUnnamedParams.size()); + for (const ParamDescriptorPtr& ptr : block_data.mUnnamedParams) + { + unnamed_handles.insert(ptr->mParamHandle); + } + for (const BlockDescriptor::param_map_t::value_type& pair : block_data.mNamedParams) { param_handle_t param_handle = pair.second->mParamHandle; @@ -255,24 +267,14 @@ namespace LLInitParam { // Ensure this param has not already been serialized // Prevents from being serialized as its own tag. - bool duplicate = false; - for (const ParamDescriptorPtr& ptr : block_data.mUnnamedParams) - { - if (param_handle == ptr->mParamHandle) - { - duplicate = true; - break; - } - } - //FIXME: for now, don't attempt to serialize values under synonyms, as current parsers // don't know how to detect them - if (duplicate) + if (unnamed_handles.find(param_handle) != unnamed_handles.end()) { continue; } - name_stack.push_back(std::make_pair(pair.first, !duplicate)); + name_stack.emplace_back(pair.first, true); const Param* diff_param = diff_block ? diff_block->getParamFromHandle(param_handle) : NULL; serialized |= serialize_func(*param, parser, name_stack, predicate_rule, diff_param); name_stack.pop_back(); @@ -300,12 +302,20 @@ namespace LLInitParam ParamDescriptor::inspect_func_t inspect_func = ptr->mInspectFunc; if (inspect_func) { - name_stack.push_back(std::make_pair("", true)); + name_stack.emplace_back("", true); inspect_func(*param, parser, name_stack, ptr->mMinCount, ptr->mMaxCount); name_stack.pop_back(); } } + // Precompute unnamed (implicit) param handles for O(1) duplicate checks. + std::unordered_set unnamed_handles; + unnamed_handles.reserve(block_data.mUnnamedParams.size()); + for (const ParamDescriptorPtr& ptr : block_data.mUnnamedParams) + { + unnamed_handles.insert(ptr->mParamHandle); + } + for(const BlockDescriptor::param_map_t::value_type& pair : block_data.mNamedParams) { param_handle_t param_handle = pair.second->mParamHandle; @@ -314,17 +324,9 @@ namespace LLInitParam if (inspect_func) { // Ensure this param has not already been inspected - bool duplicate = false; - for (const ParamDescriptorPtr &ptr : block_data.mUnnamedParams) - { - if (param_handle == ptr->mParamHandle) - { - duplicate = true; - break; - } - } + bool duplicate = unnamed_handles.find(param_handle) != unnamed_handles.end(); - name_stack.push_back(std::make_pair(pair.first, !duplicate)); + name_stack.emplace_back(pair.first, !duplicate); inspect_func(*param, parser, name_stack, pair.second->mMinCount, pair.second->mMaxCount); name_stack.pop_back(); } diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h index 95b6130bd1..0dbbc95693 100644 --- a/indra/llcommon/llinitparam.h +++ b/indra/llcommon/llinitparam.h @@ -277,8 +277,8 @@ namespace LLInitParam {} void setValueName(const std::string& key) {} - std::string getValueName() const { return ""; } - std::string calcValueName(const value_t& value) const { return ""; } + const std::string& getValueName() const { static const std::string sEmpty; return sEmpty; } + const std::string& calcValueName(const value_t& value) const { static const std::string sEmpty; return sEmpty; } void clearValueName() const {} static bool getValueFromName(const std::string& name, value_t& value) @@ -336,12 +336,12 @@ namespace LLInitParam mValueName = value_name; } - std::string getValueName() const + const std::string& getValueName() const { return mValueName; } - std::string calcValueName(const value_t& value) const + const std::string& calcValueName(const value_t& value) const { value_name_map_t* map = getValueNames(); for (typename value_name_map_t::value_type& map_pair : *map) @@ -352,7 +352,8 @@ namespace LLInitParam } } - return ""; + static const std::string sEmpty; + return sEmpty; } void clearValueName() const @@ -391,11 +392,19 @@ namespace LLInitParam static std::vector* getPossibleValues() { static std::vector sValues; + static bool sInitialized = false; - value_name_map_t* map = getValueNames(); - for (typename value_name_map_t::value_type& map_pair : *map) + // Populate once: this is called repeatedly (e.g. from inspectBlock, + // twice per param), and without a guard sValues grew unboundedly + // with duplicate entries on every call. + if (!sInitialized) { - sValues.push_back(map_pair.first); + sInitialized = true; + value_name_map_t* map = getValueNames(); + for (typename value_name_map_t::value_type& map_pair : *map) + { + sValues.push_back(map_pair.first); + } } return &sValues; } @@ -717,7 +726,7 @@ namespace LLInitParam { if (!mPtr) { - mPtr = new T(other); + mPtr = std::make_unique(other); } else { @@ -735,15 +744,15 @@ namespace LLInitParam // lazily allocate an instance of T T* ensureInstance() const { - if (mPtr == nullptr) + if (!mPtr) { - mPtr = new T(); + mPtr = std::make_unique(); } - return mPtr; + return mPtr.get(); } private: - mutable T* mPtr = nullptr; + mutable std::unique_ptr mPtr; }; // root class of all parameter blocks @@ -1069,7 +1078,7 @@ namespace LLInitParam name_stack.back().second = true; } - std::string key = typed_param.getValueName(); + const std::string& key = typed_param.getValueName(); // first try to write out name of name/value pair @@ -1086,7 +1095,7 @@ namespace LLInitParam serialized = parser.writeValue(typed_param.getValue(), name_stack); if (!serialized) { - std::string calculated_key = typed_param.calcValueName(typed_param.getValue()); + const std::string& calculated_key = typed_param.calcValueName(typed_param.getValue()); if (calculated_key.size() && (!diff_typed_param || !ParamCompare::equals(static_cast(diff_param)->getValueName(), calculated_key))) @@ -1229,7 +1238,7 @@ namespace LLInitParam name_stack.back().second = true; } - std::string key = typed_param.getValueName(); + const std::string& key = typed_param.getValueName(); if (!key.empty()) { if (!diff_param || !ParamCompare::equals(static_cast(diff_param)->getValueName(), key)) @@ -1364,7 +1373,7 @@ namespace LLInitParam mMinCount(min_count), mMaxCount(max_count) { - std::copy(value.begin(), value.end(), std::back_inserter(mValues)); + mValues = value; if (LL_UNLIKELY(block_descriptor.mInitializationState == BlockDescriptor::INITIALIZING)) { @@ -1378,14 +1387,13 @@ namespace LLInitParam bool isValid() const { size_t num_elements = numValidElements(); - return mMinCount < num_elements && num_elements < mMaxCount; + return mMinCount <= num_elements && num_elements <= mMaxCount; } static bool deserializeParam(Param& param, Parser& parser, Parser::name_stack_range_t& name_stack_range, bool new_name) { Parser::name_stack_range_t new_name_stack_range(name_stack_range); self_t& typed_param = static_cast(param); - value_t value; // pop first element if empty string if (new_name_stack_range.first != new_name_stack_range.second && new_name_stack_range.first->first.empty()) @@ -1398,20 +1406,27 @@ namespace LLInitParam { std::string name; + // add a new element and parse directly into it, rather than + // parsing into a local and copying it into the container + named_value_t& new_value = typed_param.mValues.emplace_back(value_t()); + // try to parse a known named value if(named_value_t::valueNamesExist() && parser.readValue(name) - && named_value_t::getValueFromName(name, value)) + && named_value_t::getValueFromName(name, new_value.getValue())) { - typed_param.add(value); - typed_param.mValues.back().setValueName(name); + typed_param.setProvided(); + new_value.setValueName(name); return true; } - else if (parser.readValue(value)) // attempt to read value directly + else if (parser.readValue(new_value.getValue())) // attempt to read value directly { - typed_param.add(value); + typed_param.setProvided(); return true; } + + // failed to parse a value; remove the element we added + typed_param.mValues.pop_back(); } return false; } @@ -1434,8 +1449,8 @@ namespace LLInitParam it != end_it; ++it) { - std::string key = it->getValueName(); - name_stack.push_back(std::make_pair(std::string(), true)); + const std::string& key = it->getValueName(); + name_stack.emplace_back(std::string(), true); if(key.empty()) // not parsed via name values, write out value directly @@ -1443,7 +1458,7 @@ namespace LLInitParam bool value_written = parser.writeValue(*it, name_stack); if (!value_written) { - std::string calculated_key = it->calcValueName(it->getValue()); + const std::string& calculated_key = it->calcValueName(it->getValue()); if (parser.writeValue(calculated_key, name_stack)) { serialized = true; @@ -1485,6 +1500,12 @@ namespace LLInitParam setProvided(flag_as_provided); } + void set(container_t&& val, bool flag_as_provided = true) + { + mValues = std::move(val); + setProvided(flag_as_provided); + } + param_value_t& add() { mValues.push_back(value_t()); @@ -1494,7 +1515,7 @@ namespace LLInitParam self_t& add(const value_t& item) { - mValues.push_back(item); + mValues.emplace_back(item); setProvided(); return *this; } @@ -1596,7 +1617,7 @@ namespace LLInitParam mMinCount(min_count), mMaxCount(max_count) { - std::copy(value.begin(), value.end(), back_inserter(mValues)); + mValues = value; if (LL_UNLIKELY(block_descriptor.mInitializationState == BlockDescriptor::INITIALIZING)) { @@ -1609,7 +1630,7 @@ namespace LLInitParam bool isValid() const { size_t num_elements = numValidElements(); - return mMinCount < num_elements && num_elements < mMaxCount; + return mMinCount <= num_elements && num_elements <= mMaxCount; } @@ -1689,9 +1710,9 @@ namespace LLInitParam it != end_it; ++it) { - name_stack.push_back(std::make_pair(std::string(), true)); + name_stack.emplace_back(std::string(), true); - std::string key = it->getValueName(); + const std::string& key = it->getValueName(); if (!key.empty()) { serialized |= parser.writeValue(key, name_stack); @@ -1735,6 +1756,12 @@ namespace LLInitParam setProvided(flag_as_provided); } + void set(container_t&& val, bool flag_as_provided = true) + { + mValues = std::move(val); + setProvided(flag_as_provided); + } + param_value_t& add() { mValues.push_back(value_t()); @@ -1744,7 +1771,7 @@ namespace LLInitParam self_t& add(const value_t& item) { - mValues.push_back(item); + mValues.emplace_back(item); setProvided(); return *this; } @@ -2116,12 +2143,24 @@ namespace LLInitParam return *this; } + Multiple& operator =(container_t&& val) + { + super_t::set(std::move(val)); + return *this; + } + DERIVED_BLOCK& operator()(const container_t& val) { super_t::set(val); return static_cast(Param::enclosingBlock()); } + DERIVED_BLOCK& operator()(container_t&& val) + { + super_t::set(std::move(val)); + return static_cast(Param::enclosingBlock()); + } + static bool validate(const Param* paramp) { size_t num_valid = ((super_t*)paramp)->numValidElements(); @@ -2686,7 +2725,7 @@ namespace LLInitParam const derived_t& typed_param = static_cast(*this); const derived_t* diff_param = static_cast(diff_block); - //std::string key = typed_param.getValueName(); + //const std::string& key = typed_param.getValueName(); //// first try to write out name of name/value pair //if (!key.empty()) From 3a00b95056cac0451f6eeb170725e75b8b49a0b5 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 14 Jun 2026 02:10:57 -0400 Subject: [PATCH 9/9] llxml: address review - guard mCurrent, use %u for U32 sscanf CodeRabbit review on #313: - LLXmlTreeParser::characterData/endElement dereferenced mCurrent without a null check. It is structurally non-null for well-formed input (expat routes prolog/epilog text to the default handler, and endElement only fires for an opened element), but guard both as cheap defensive hardening. - StartXMLNode parsed version/length/precision into U32 with %d (expects int*); switch to %u to match the target type. Output is unchanged for the non-negative values these attributes hold. Co-Authored-By: Claude Opus 4.8 (1M context) --- indra/llxml/llxmlnode.cpp | 6 +++--- indra/llxml/llxmltree.cpp | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/indra/llxml/llxmlnode.cpp b/indra/llxml/llxmlnode.cpp index 64e3b2eb37..67c22c8955 100644 --- a/indra/llxml/llxmlnode.cpp +++ b/indra/llxml/llxmlnode.cpp @@ -365,7 +365,7 @@ void XMLCALL StartXMLNode(void *userData, { U32 version_major = 0; U32 version_minor = 0; - if (sscanf(atts[pos + 1], "%d.%d", &version_major, &version_minor) > 0) + if (sscanf(atts[pos + 1], "%u.%u", &version_major, &version_minor) > 0) { new_node->mVersionMajor = version_major; new_node->mVersionMinor = version_minor; @@ -374,7 +374,7 @@ void XMLCALL StartXMLNode(void *userData, else if (('s' == attr_name[0] && "size" == attr_name) || ('l' == attr_name[0] && "length" == attr_name)) { U32 length; - if (sscanf(atts[pos + 1], "%d", &length) > 0) + if (sscanf(atts[pos + 1], "%u", &length) > 0) { new_node->mLength = length; } @@ -382,7 +382,7 @@ void XMLCALL StartXMLNode(void *userData, else if ('p' == attr_name[0] && "precision" == attr_name) { U32 precision; - if (sscanf(atts[pos + 1], "%d", &precision) > 0) + if (sscanf(atts[pos + 1], "%u", &precision) > 0) { new_node->mPrecision = precision; } diff --git a/indra/llxml/llxmltree.cpp b/indra/llxml/llxmltree.cpp index d0bc78d460..eff2aabf3e 100644 --- a/indra/llxml/llxmltree.cpp +++ b/indra/llxml/llxmltree.cpp @@ -601,6 +601,11 @@ void LLXmlTreeParser::endElement(const char* name) LL_INFOS() << tabs() << "endElement " << name << LL_ENDL; } + if( !mCurrent ) + { + return; + } + if( !mCurrent->mContents.empty() ) { LLStringUtil::trim(mCurrent->mContents); @@ -612,7 +617,7 @@ void LLXmlTreeParser::endElement(const char* name) void LLXmlTreeParser::characterData(const char *s, int len) { - if (!s || len <= 0) + if (!s || len <= 0 || !mCurrent) { return; }