From f8d0be23b3e46c78ece76ca7726a294f39157ace Mon Sep 17 00:00:00 2001 From: howsohazard <143410553+howsohazard@users.noreply.github.com> Date: Fri, 26 Jan 2024 14:22:09 -0500 Subject: [PATCH] 19116: Fixes bug where string intern values were swapped in rare circumstances, leading to instability (#65) --- src/Amalgam/AssetManager.cpp | 2 +- src/Amalgam/GeneralizedDistance.h | 4 +- src/Amalgam/Parser.cpp | 4 +- src/Amalgam/SBFDSColumnData.h | 4 +- src/Amalgam/entity/Entity.h | 10 ++--- src/Amalgam/evaluablenode/EvaluableNode.cpp | 45 ++----------------- src/Amalgam/evaluablenode/EvaluableNode.h | 15 +++---- .../EvaluableNodeTreeManipulation.cpp | 8 ++-- .../EvaluableNodeTreeManipulation.h | 6 +-- src/Amalgam/importexport/FileSupportJSON.cpp | 6 +-- src/Amalgam/importexport/FileSupportYAML.cpp | 6 +-- .../InterpreterOpcodesDataTypes.cpp | 4 +- .../InterpreterOpcodesEntityControl.cpp | 2 +- .../InterpreterOpcodesListManipulation.cpp | 2 +- .../InterpreterOpcodesTransformations.cpp | 6 +-- src/Amalgam/string/StringInternPool.cpp | 2 +- src/Amalgam/string/StringInternPool.h | 8 ++-- 17 files changed, 47 insertions(+), 87 deletions(-) diff --git a/src/Amalgam/AssetManager.cpp b/src/Amalgam/AssetManager.cpp index 1a00d41e..96d9dd96 100644 --- a/src/Amalgam/AssetManager.cpp +++ b/src/Amalgam/AssetManager.cpp @@ -432,7 +432,7 @@ std::string AssetManager::GetEvaluableNodeSourceFromComments(EvaluableNode *en) { if(en->HasComments()) { - auto &comment = en->GetCommentsString(); + auto comment = en->GetCommentsString(); auto first_line_end = comment.find('\n'); if(first_line_end == std::string::npos) source = comment; diff --git a/src/Amalgam/GeneralizedDistance.h b/src/Amalgam/GeneralizedDistance.h index cc8e0643..af6b80bf 100644 --- a/src/Amalgam/GeneralizedDistance.h +++ b/src/Amalgam/GeneralizedDistance.h @@ -795,8 +795,8 @@ class GeneralizedDistance { if(a_type == ENIVT_STRING_ID && b_type == ENIVT_STRING_ID) { - auto &a_str = string_intern_pool.GetStringFromID(a.stringID); - auto &b_str = string_intern_pool.GetStringFromID(b.stringID); + auto a_str = string_intern_pool.GetStringFromID(a.stringID); + auto b_str = string_intern_pool.GetStringFromID(b.stringID); return static_cast(EvaluableNodeTreeManipulation::EditDistance(a_str, b_str)); } diff --git a/src/Amalgam/Parser.cpp b/src/Amalgam/Parser.cpp index 5cbcf13b..45ccd6ca 100644 --- a/src/Amalgam/Parser.cpp +++ b/src/Amalgam/Parser.cpp @@ -705,7 +705,7 @@ void Parser::AppendAssocKeyValuePair(UnparseData &upd, StringInternPool::StringI else upd.result.push_back(' '); - const std::string &key_str = string_intern_pool.GetStringFromID(key_sid); + auto key_str = string_intern_pool.GetStringFromID(key_sid); //surround in quotes only if needed if(key_sid != string_intern_pool.NOT_A_STRING_ID @@ -805,7 +805,7 @@ void Parser::Unparse(UnparseData &upd, EvaluableNode *tree, EvaluableNode *paren { upd.result.push_back('"'); - auto &s = tree->GetStringValue(); + auto s = tree->GetStringValue(); if(NeedsBackslashify(s)) upd.result.append(Backslashify(s)); else diff --git a/src/Amalgam/SBFDSColumnData.h b/src/Amalgam/SBFDSColumnData.h index 21f0fec9..a55f1ca9 100644 --- a/src/Amalgam/SBFDSColumnData.h +++ b/src/Amalgam/SBFDSColumnData.h @@ -704,7 +704,7 @@ class SBFDSColumnData // and adding all the new ones if(value_type == ENIVT_STRING_ID) { - auto &s = string_intern_pool.GetStringFromID(value.stringID); + auto s = string_intern_pool.GetStringFromID(value.stringID); return static_cast(longestStringLength + StringManipulation::GetNumUTF8Characters(s)); } else if(value_type == ENIVT_NULL) @@ -1137,7 +1137,7 @@ class SBFDSColumnData //updates longestStringLength and indexWithLongestString based on parameters inline void UpdateLongestString(StringInternPool::StringID sid, size_t index) { - auto &str = string_intern_pool.GetStringFromID(sid); + auto str = string_intern_pool.GetStringFromID(sid); size_t str_size = StringManipulation::GetUTF8CharacterLength(str); if(str_size > longestStringLength) { diff --git a/src/Amalgam/entity/Entity.h b/src/Amalgam/entity/Entity.h index 65652f50..27d4a899 100644 --- a/src/Amalgam/entity/Entity.h +++ b/src/Amalgam/entity/Entity.h @@ -200,7 +200,7 @@ class Entity void RebuildLabelIndex(); //Returns the id for this Entity - inline const std::string &GetId() + inline const std::string GetId() { return string_intern_pool.GetStringFromID(GetIdStringId()); } @@ -387,7 +387,7 @@ class Entity inline static bool IsNamedEntity(StringInternPool::StringID id) { - const std::string &id_name = string_intern_pool.GetStringFromID(id); + auto id_name = string_intern_pool.GetStringFromID(id); if(id_name == StringInternPool::EMPTY_STRING) return false; return IsNamedEntity(id_name); @@ -436,7 +436,7 @@ class Entity if(label_sid == string_intern_pool.NOT_A_STRING_ID) return false; - const std::string &label_name = string_intern_pool.GetStringFromID(label_sid); + auto label_name = string_intern_pool.GetStringFromID(label_sid); return IsLabelValidAndPublic(label_name); } @@ -455,7 +455,7 @@ class Entity //returns true if the label is only accessible to itself (starts with !) static inline bool IsLabelPrivate(StringInternPool::StringID label_sid) { - const std::string &label_name = string_intern_pool.GetStringFromID(label_sid); + auto label_name = string_intern_pool.GetStringFromID(label_sid); return IsLabelPrivate(label_name); } @@ -472,7 +472,7 @@ class Entity //returns true if the label is accessible to contained entities (starts with ^) static inline bool IsLabelAccessibleToContainedEntities(StringInternPool::StringID label_sid) { - const std::string &label_name = string_intern_pool.GetStringFromID(label_sid); + auto label_name = string_intern_pool.GetStringFromID(label_sid); return IsLabelAccessibleToContainedEntities(label_name); } diff --git a/src/Amalgam/evaluablenode/EvaluableNode.cpp b/src/Amalgam/evaluablenode/EvaluableNode.cpp index 8122ebf0..6036fa39 100644 --- a/src/Amalgam/evaluablenode/EvaluableNode.cpp +++ b/src/Amalgam/evaluablenode/EvaluableNode.cpp @@ -124,43 +124,6 @@ bool EvaluableNode::IsTrue(EvaluableNode *n) return true; } -EvaluableNode *EvaluableNode::RetrieveImmediateAssocValue(EvaluableNode *n, const std::string &key) -{ - if(!IsAssociativeArray(n)) - return nullptr; - - StringInternPool::StringID key_sid = string_intern_pool.GetIDFromString(key); - if(key_sid == StringInternPool::NOT_A_STRING_ID) - return nullptr; - - //first try for mapped - if(n->IsAssociativeArray()) - { - auto &mcn = n->GetMappedChildNodesReference(); - auto result_in_mapped = mcn.find(key_sid); - if(result_in_mapped != end(mcn)) - return result_in_mapped->second; - - //not found - return nullptr; - } - - //try for uninterpreted, every other value is a key, so skip values and make sure have room for the last key - auto &ocn = n->GetOrderedChildNodes(); - for(size_t i = 0; i + 1 < ocn.size(); i += 2) - { - EvaluableNode *key_node = ocn[i]; - if(key_node == nullptr) - continue; - if(key_node->GetType() != ENT_STRING) - continue; - if(key_node->GetStringValue() == key) - return ocn[i + 1]; - } - - return nullptr; -} - int EvaluableNode::Compare(EvaluableNode *a, EvaluableNode *b) { //try numerical comparison first @@ -221,7 +184,7 @@ double EvaluableNode::ToNumber(EvaluableNode *e, double value_if_null) auto sid = e->GetStringIDReference(); if(sid == string_intern_pool.NOT_A_STRING_ID) return value_if_null; - const auto &str = string_intern_pool.GetStringFromID(sid); + auto str = string_intern_pool.GetStringFromID(sid); auto [value, success] = Platform_StringToNumber(str); if(success) return value; @@ -730,7 +693,7 @@ void EvaluableNode::SetStringID(StringInternPool::StringID id) } } -const std::string &EvaluableNode::GetStringValue() +std::string EvaluableNode::GetStringValue() { if(DoesEvaluableNodeTypeUseStringData(GetType())) { @@ -905,7 +868,7 @@ size_t EvaluableNode::GetNumLabels() return sids.size(); } -const std::string &EvaluableNode::GetLabel(size_t label_index) +std::string EvaluableNode::GetLabel(size_t label_index) { if(!HasExtendedValue()) { @@ -1056,7 +1019,7 @@ std::vector EvaluableNode::GetCommentsSeparateLines() if(comment_sid <= StringInternPool::EMPTY_STRING_ID) return comment_lines; - const auto &full_comments = string_intern_pool.GetStringFromID(comment_sid); + auto full_comments = string_intern_pool.GetStringFromID(comment_sid); //early exit if(full_comments == "") diff --git a/src/Amalgam/evaluablenode/EvaluableNode.h b/src/Amalgam/evaluablenode/EvaluableNode.h index c4567cfd..e0d35d75 100644 --- a/src/Amalgam/evaluablenode/EvaluableNode.h +++ b/src/Amalgam/evaluablenode/EvaluableNode.h @@ -254,11 +254,6 @@ class EvaluableNode return (n != nullptr && IsEvaluableNodeTypeQuery(n->GetType())); } - //Returns the value requested from an associative array regardless of whether the associative array has been interpreted - // into mappedChildNodes or is still in orderedChildNodes - //returns nullptr if not found - static EvaluableNode *RetrieveImmediateAssocValue(EvaluableNode *n, const std::string &key); - //Returns positive if a is less than b, // negative if greater, or 0 if equal or not numerically comparable static int Compare(EvaluableNode *a, EvaluableNode *b); @@ -477,7 +472,7 @@ class EvaluableNode return StringInternPool::NOT_A_STRING_ID; } void SetStringID(StringInternPool::StringID id); - const std::string &GetStringValue(); + std::string GetStringValue(); void SetStringValue(const std::string &v); //gets the string ID and clears the node's string ID, but does not destroy the string reference, // leaving the reference handling up to the caller @@ -491,7 +486,7 @@ class EvaluableNode std::vector GetLabelsStrings(); void SetLabelsStringIds(const std::vector &label_string_ids); size_t GetNumLabels(); - const std::string &GetLabel(size_t label_index); + std::string GetLabel(size_t label_index); const StringInternPool::StringID GetLabelStringId(size_t label_index); void RemoveLabel(size_t label_index); void ClearLabels(); @@ -504,7 +499,7 @@ class EvaluableNode //functions for getting and setting node comments by string or by StringID // all Comment functions perform any reference counting management necessary when setting and clearing StringInternPool::StringID GetCommentsStringId(); - inline const std::string &GetCommentsString() + inline std::string GetCommentsString() { return string_intern_pool.GetStringFromID(GetCommentsStringId()); } @@ -1135,7 +1130,7 @@ class EvaluableNodeImmediateValueWithType if(nodeValue.stringID == string_intern_pool.NOT_A_STRING_ID) return value_if_null; - const auto &str = string_intern_pool.GetStringFromID(nodeValue.stringID); + auto str = string_intern_pool.GetStringFromID(nodeValue.stringID); auto [value, success] = Platform_StringToNumber(str); if(success) return value; @@ -1164,7 +1159,7 @@ class EvaluableNodeImmediateValueWithType if(nodeValue.stringID == string_intern_pool.NOT_A_STRING_ID) return std::make_pair(false, ""); - const auto &str = string_intern_pool.GetStringFromID(nodeValue.stringID); + auto str = string_intern_pool.GetStringFromID(nodeValue.stringID); return std::make_pair(true, str); } diff --git a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp index 527e80e9..4353025c 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp +++ b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp @@ -78,8 +78,8 @@ inline StringInternPool::StringID MixStringValues(StringInternPool::StringID a, if(b == StringInternPool::NOT_A_STRING_ID) return string_intern_pool.CreateStringReference(a); - const auto &a_str = string_intern_pool.GetStringFromID(a); - const auto &b_str = string_intern_pool.GetStringFromID(b); + auto a_str = string_intern_pool.GetStringFromID(a); + auto b_str = string_intern_pool.GetStringFromID(b); std::string result = EvaluableNodeTreeManipulation::MixStrings(a_str, b_str, random_stream, fraction_a, fraction_b); @@ -1067,7 +1067,7 @@ bool EvaluableNodeTreeManipulation::CollectLabelIndexesFromTree(EvaluableNode *t for(size_t i = 0; i < num_labels; i++) { auto label_sid = tree->GetLabelStringId(i); - const std::string &label_name = string_intern_pool.GetStringFromID(label_sid); + auto label_name = string_intern_pool.GetStringFromID(label_sid); if(label_name.size() == 0) continue; @@ -1122,7 +1122,7 @@ bool EvaluableNodeTreeManipulation::CollectLabelIndexesFromTreeAndMakeLabelNorma for(size_t i = 0; i < num_labels; i++) { auto label_sid = tree->GetLabelStringId(i); - const std::string &label_name = string_intern_pool.GetStringFromID(label_sid); + auto label_name = string_intern_pool.GetStringFromID(label_sid); if(label_name.size() == 0) continue; diff --git a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h index e34be9e9..f2b21a11 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h +++ b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h @@ -345,7 +345,7 @@ class EvaluableNodeTreeManipulation } //returns the commonality between two strings that are different - static constexpr double CommonalityBetweenStrings(StringInternPool::StringID sid1, StringInternPool::StringID sid2) + static inline double CommonalityBetweenStrings(StringInternPool::StringID sid1, StringInternPool::StringID sid2) { if(sid1 == sid2) return 1.0; @@ -354,8 +354,8 @@ class EvaluableNodeTreeManipulation if(sid1 == string_intern_pool.NOT_A_STRING_ID || sid2 == string_intern_pool.NOT_A_STRING_ID) return 0.125; - const auto &s1 = string_intern_pool.GetStringFromID(sid1); - const auto &s2 = string_intern_pool.GetStringFromID(sid2); + auto s1 = string_intern_pool.GetStringFromID(sid1); + auto s2 = string_intern_pool.GetStringFromID(sid2); size_t s1_len = 0; size_t s2_len = 0; diff --git a/src/Amalgam/importexport/FileSupportJSON.cpp b/src/Amalgam/importexport/FileSupportJSON.cpp index 128bfb0e..305ba964 100644 --- a/src/Amalgam/importexport/FileSupportJSON.cpp +++ b/src/Amalgam/importexport/FileSupportJSON.cpp @@ -155,7 +155,7 @@ bool EvaluableNodeToJsonStringRecurse(EvaluableNode *en, std::string &json_str, else first_cn = false; - const auto &str = string_intern_pool.GetStringFromID(cn_id); + auto str = string_intern_pool.GetStringFromID(cn_id); EscapeAndAppendStringToJsonString(str, json_str); json_str += ':'; @@ -185,7 +185,7 @@ bool EvaluableNodeToJsonStringRecurse(EvaluableNode *en, std::string &json_str, if(i > 0) json_str += ','; - const auto &str = string_intern_pool.GetStringFromID(key_sids[i]); + auto str = string_intern_pool.GetStringFromID(key_sids[i]); EscapeAndAppendStringToJsonString(str, json_str); json_str += ':'; @@ -266,7 +266,7 @@ bool EvaluableNodeToJsonStringRecurse(EvaluableNode *en, std::string &json_str, } else { - const auto &str_value = en->GetStringValue(); + auto str_value = en->GetStringValue(); EscapeAndAppendStringToJsonString(str_value, json_str); } } diff --git a/src/Amalgam/importexport/FileSupportYAML.cpp b/src/Amalgam/importexport/FileSupportYAML.cpp index 52e56cb9..7233751f 100644 --- a/src/Amalgam/importexport/FileSupportYAML.cpp +++ b/src/Amalgam/importexport/FileSupportYAML.cpp @@ -72,7 +72,7 @@ bool EvaluableNodeToYamlStringRecurse(EvaluableNode *en, ryml::NodeRef &built_el { for(auto &[cn_id, cn] : mcn) { - const auto &str = string_intern_pool.GetStringFromID(cn_id); + auto str = string_intern_pool.GetStringFromID(cn_id); auto new_element = built_element.append_child(); new_element << ryml::key(str); if(!EvaluableNodeToYamlStringRecurse(cn, new_element, sort_keys)) @@ -92,7 +92,7 @@ bool EvaluableNodeToYamlStringRecurse(EvaluableNode *en, ryml::NodeRef &built_el { auto k = mcn.find(key_sids[i]); - const auto &str = string_intern_pool.GetStringFromID(k->first); + auto str = string_intern_pool.GetStringFromID(k->first); auto new_element = built_element.append_child(); new_element << ryml::key(str); @@ -141,7 +141,7 @@ bool EvaluableNodeToYamlStringRecurse(EvaluableNode *en, ryml::NodeRef &built_el } else { - auto &str_value = en->GetStringValue(); + auto str_value = en->GetStringValue(); built_element << str_value; } } diff --git a/src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp b/src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp index a3ed7005..d0fb1236 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp @@ -539,7 +539,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_FORMAT(EvaluableNode *en, } else //need to parse the string { - const auto &from_type_str = string_intern_pool.GetStringFromID(from_type); + auto from_type_str = string_intern_pool.GetStringFromID(from_type); //see if it starts with the date string if(from_type_str.compare(0, date_string.size(), date_string) == 0) @@ -862,7 +862,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_FORMAT(EvaluableNode *en, } else //need to parse the string { - const auto &to_type_str = string_intern_pool.GetStringFromID(to_type); + auto to_type_str = string_intern_pool.GetStringFromID(to_type); //if it starts with the date string if(to_type_str.compare(0, date_string.size(), date_string) == 0) diff --git a/src/Amalgam/interpreter/InterpreterOpcodesEntityControl.cpp b/src/Amalgam/interpreter/InterpreterOpcodesEntityControl.cpp index 2bea2a97..ca4aca5f 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesEntityControl.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesEntityControl.cpp @@ -356,7 +356,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CREATE_ENTITIES(EvaluableN if(!AllowUnlimitedExecutionNodes()) curNumExecutionNodesAllocatedToEntities += new_entity->GetDeepSizeInNodes(); - const std::string &new_entity_id_string = string_intern_pool.GetStringFromID(new_entity_id); + auto new_entity_id_string = string_intern_pool.GetStringFromID(new_entity_id); new_entity->SetRandomState(destination_entity_parent->CreateRandomStreamFromStringAndRand(new_entity_id_string), false); destination_entity_parent->AddContainedEntityViaReference(new_entity, new_entity_id, writeListeners); diff --git a/src/Amalgam/interpreter/InterpreterOpcodesListManipulation.cpp b/src/Amalgam/interpreter/InterpreterOpcodesListManipulation.cpp index a2ed7f9e..68a37a86 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesListManipulation.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesListManipulation.cpp @@ -536,7 +536,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SIZE(EvaluableNode *en, bo { if(cur->GetType() == ENT_STRING) { - const auto &s = cur->GetStringValue(); + auto s = cur->GetStringValue(); size = StringManipulation::GetNumUTF8Characters(s); } else diff --git a/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp b/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp index 7fa90385..e5b1bb3a 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp @@ -793,8 +793,8 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_APPLY(EvaluableNode *en, b { if(type_node->GetType() == ENT_STRING) { - auto &new_type_string = type_node->GetStringValue(); - new_type = GetEvaluableNodeTypeFromString(new_type_string, true); + auto new_type_sid = type_node->GetStringIDReference(); + new_type = GetEvaluableNodeTypeFromStringId(new_type_sid); evaluableNodeManager->FreeNodeTreeIfPossible(type_node); } else @@ -1097,7 +1097,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CONTAINS_VALUE(EvaluableNo else if(container->GetType() == ENT_STRING && !EvaluableNode::IsEmptyNode(value)) { //compute regular expression - const std::string &s = container->GetStringValue(); + std::string s = container->GetStringValue(); std::string value_as_str = EvaluableNode::ToStringPreservingOpcodeType(value); diff --git a/src/Amalgam/string/StringInternPool.cpp b/src/Amalgam/string/StringInternPool.cpp index 37790b66..aef45de8 100644 --- a/src/Amalgam/string/StringInternPool.cpp +++ b/src/Amalgam/string/StringInternPool.cpp @@ -3,7 +3,7 @@ StringInternPool string_intern_pool; -const std::string &StringInternPool::GetStringFromID(StringInternPool::StringID id) +const std::string StringInternPool::GetStringFromID(StringInternPool::StringID id) { #if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE) Concurrency::ReadLock lock(sharedMutex); diff --git a/src/Amalgam/string/StringInternPool.h b/src/Amalgam/string/StringInternPool.h index e09c3693..80849fa0 100644 --- a/src/Amalgam/string/StringInternPool.h +++ b/src/Amalgam/string/StringInternPool.h @@ -31,7 +31,9 @@ class StringInternPool } //translates the id to a string, empty string if it does not exist - const std::string &GetStringFromID(StringID id); + //because a flat hash map is used as the storage container, it is possible that any allocation or deallocation + //may invalidate the location, so a copy must be made to return the value + const std::string GetStringFromID(StringID id); //translates the string to the corresponding ID, 0 is the empty string, maximum value of size_t means it does not exist StringID GetIDFromString(const std::string &str); @@ -391,7 +393,7 @@ class StringInternRef } //allow being able to use as a string - inline operator const std::string &() + inline operator const std::string () { return string_intern_pool.GetStringFromID(id); } @@ -464,7 +466,7 @@ class StringInternWeakRef } //allow being able to use as a string - inline operator const std::string &() + inline operator const std::string () { return string_intern_pool.GetStringFromID(id); }