19116: Fixes bug where string intern values were swapped in rare circ…

…umstances, leading to instability (#65)
howsoai · Jan 26, 2024 · f8d0be2 · f8d0be2
1 parent 63be16b
commit f8d0be2
Show file tree

Hide file tree

Showing 17 changed files with 47 additions and 87 deletions.
diff --git 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
@@ -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<double>(EvaluableNodeTreeManipulation::EditDistance(a_str, b_str));
 			}
 

diff --git 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
@@ -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<double>(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
@@ -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
@@ -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<std::string> 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
@@ -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<std::string> GetLabelsStrings();
 	void SetLabelsStringIds(const std::vector<StringInternPool::StringID> &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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);